Expand Supports syntax (part deux)

[kbrock]

Part of project: #21179

Also merge: ManageIQ/manageiq-ui-classic#8366

This is based upon a conversation started by @chessbyte : #21269 which started in vmware-provider

It came up a few times and now that we have reworked supports a bit, it seemed like a good time to see revisit and see if this new syntax makes more sense.

Ideas in discussion

Before

    supports :check_compliance
    supports_not :check_compliance
    supports_not :check_compliance, :reason => "Feature not available/supported"

    supports :check_compliance do
      unsupported_reason_add(:check_compliance, "No compliance policies assigned") unless has_compliance_policies?
    end

Most of the supports_not blocks have been transitioned to use the default reason:

#	comment
245	supports :feature
151	supports_not :feature
23	supports_not :feature, :reason => ""
169	supports :feature do ; end

String Syntax

    supports :check_compliance do
      "No compliance policies assigned" unless has_compliance_policies?
    end

This gives the end user more of an understanding as to why this operation is not available.
Half of the time this is an if/elsif chain.
Returning nil means that this feature is supported.

Of the reasons given, a quarter could just use the default value:

%	comment
50	multiple reasons per block
25	nondescript single reason
25	single reason

Boolean Syntax

We have dropped the use of returning a boolean. Most of those cases have been converted to the standard supports / supports_not syntax.

[kbrock]

extra note:

I am wondering if we want a chain method, to base the supports either upon a parent feature or upon another feature. host, vm, and vm_or_template all base one supports upon a core feature.

supports :stop do
  if !supports?(:control)
    unsupported_reason(:control)
  elsif !vm_powered_on?
    _('The VM is not powered on')
  end
end
#
chain_supports(:stop, :control) { _('The VM is not powered on') unless vm_powered_on? }
#
supports(:stop => :control) { _('The VM is not powered on') unless vm_powered_on? }

The two are more compact, but they don't necessarily read better.
Also, they can depend upon 2. My imagined code behind this feels unwieldy, and I don't think it makes the code base any more readable

---

There are a few more ways to chain that seemed decent:

supports(:reset) { unsupported_reason(:control) }

supports :stop do
  unsupported_reason(:control) || vm_powered_on?
end

Surprisingly, mixing Strings and true/false seem to work fine, but possibly that is just begging for late night bugs.

[kbrock]

Implementing the boolean or string logic looks relatively straight forward:

if !unsupported.key?(feature) && (result == false || result.kind_of?(String))
  unsupported_reason_add(feature, reason_or_default(result))
end

==> moved into code to see if we like this or not

[kbrock]

cops did not update to new code
I'm finding that just using this for a day has given me new ideas to simplify the implementation.

push(es):

• fixed use case for strings
• added boolean case (and re-pushed a few times)

[kbrock]

kicking cops

[kbrock]

WIP:

• @Fryguy and possibly some others want to give this a little time to come up with better ideas

[Fryguy]

Ok, sorry it took me some time to formulate this, but my fundamental problem with the approach is what you actually already laid out in the Implementation section of the OP. The return value from the block has multiple truthiness values that are contradictory and it's confusing to read. I see you wrote that it's not, but to me it's very confusing.

---

Here's what I had started writing before reading your comment there, but it's a similar table:

Given the word supports is a positive word in contrast to supports_not which I consider negative, and truthiness-wise I consider true to be a positive word and false/nil to be a negative word, you get this:

code	positivity	supported?
supports { true }	positive { positive }	yes
support { "xxx" }	postive { positive }	no
supports { false }	positive { negative }	no
supports { nil }	positive { negative }	yes
supports	positive	yes

---

I'll leave it up to @agrare to decide though, since this code falls almost entirely in the providers space.

[kbrock]

I almost did not introduce the true vs false, concept because it seemed too complicated. You didn't like the way I described the issue and wrote it out again. (the very fact that you did this shows us that this concept is confusing)

But in the end of the day, the most important thing is to read the models and decide if it is clear.

For me, the new way was easy to read, concise, and even mixing strings and booleans were clear.

[Fryguy]

Curious this is not i18n'd...we can do that in a follow up.

[kbrock]

we have a lot of these that are not internationalized''

[kbrock]

update:

• rebased to fix build failure
• moved TODOs to separate issue Use supports in more instances #21989

[kbrock]

update:

• rebase to fix merge conflict

[kbrock]

We have dropped the case for boolean.
I decided to include the text here for future reference

REDACTED: true false syntax

    supports(:check_compliance:) { has_compliance_policies? }

true returned means that this DOES support compliance
false return means that this does not support compliance.

REDACTED: Implementation

I added this as the last commit. The implementation is simple enough, but it is a little ambiguous/confusing if you think hard enough (too hard?).

block return value	logical value	supported?
true	true	yes
false	false	no
String	true	no
nil	false	yes

The fact that a logical true can mean supported or not is a little confusing, and makes this option a little error prone. But in practice the code reads well enough.

Introducing the boolean did simplify a few in core, and many more in the associated providers.

Current implementation

As mentioned above, it was simpler to just remove the need for these cases.

[kbrock]

anyway
TL;DR: currently only change is it allows you to return a string rather than calling unsupported_reason_add

[agrare]

@miq-bot cross-repo-tests /all

[Fryguy]

Fantastic work!

[kbrock]

Thanks for the x-repo @agrare
It looks like the failure is the ttl issue with the API.

Can we merge this with that sporadic failure?

[agrare]

@kbrock yeah API looks sporadic and ui-service failure looks unrelated (npm) but the ui-classic failure looks like it might be related https://github.com/ManageIQ/manageiq-cross_repo-tests/runs/7414529833?check_suite_focus=true#step:6:2847

[kbrock]

update:

• swapped order of snapshot checks, same answers, but messages are a little different
• note: still using default error message when snapshot is not supported (vs custom error before) - so it requires a merge over there

[miq-bot]

Checked commits kbrock/manageiq@ac917f0~...783ced7 with ruby 2.6.9, rubocop 1.19.1, haml-lint 0.35.0, and yamllint
14 files checked, 0 offenses detected
Everything looks fine. ⭐

[kbrock]

@miq-bot cross-repo-tests ManageIQ/manageiq-ui-classic#8366

[chessbyte]

Glad to see this over the finish line!