(PUP-5741) Fix confusing errors when comparing values #4597
Conversation
| @@ -130,7 +135,7 @@ def self.hard_issue(issue_code, *args, &block) | |||
| # @todo configuration | |||
| # | |||
| NAME_WITH_HYPHEN = issue :NAME_WITH_HYPHEN, :name do | |||
| "#{label.a_an_uc(semantic)} may not have a name containing a hyphen. The name '#{name}' is not legal" | |||
| "#{provider.a_an_uc(semantic)} may not have a name containing a hyphen. The name '#{name}' is not legal" | |||
There was a problem hiding this comment.
We could just make a_an, a_an_uc, the, and the_uc available directly. Here that would be:
"#{a_an_uc(semantic)} may not have a name containing a hyphen. The name '#{name}' is not legal"
There was a problem hiding this comment.
We could, but that's not within scope for this PR IMO.
There was a problem hiding this comment.
The "provider.a_an_uc" does not read nicely, hence my suggestion to get rid of it completely. I think that is acceptable to do within the scope of this ticket. It is a very small additional change.
thallgren
force-pushed
the
issue/pup-5741/confusing-errors
branch
from
fbc00be
to
622bc7f
Compare
| @@ -58,7 +58,8 @@ def format(hash ={}) | |||
| # in the issue's message producing block. | |||
| # @api private | |||
| # | |||
| class MessageData | |||
| class MessageData < Puppet::Pops::LabelProvider | |||
There was a problem hiding this comment.
Yeah, so that is a quick way of getting the methods. But it is not right :-)
This class is not allowed to have those methods itself as it is language agnostic. It is the label provider's job to do that. We happen to have an english provider that has a base implementation for all different kinds of english label providers.
The support should be implemented as delegation to the label provider that is referencing.
There was a problem hiding this comment.
i.e. this class must have the mehods, and they must then call the same method on the provider.
There was a problem hiding this comment.
The I18N implementation is only embryonic so hard to guess this from just looking at the code.
There was a problem hiding this comment.
You can do a method missing, and delegate to the provider if it is set, and it it responds to the missing method.
There was a problem hiding this comment.
Regarding delegation. Delegating will not make a future I18N transition easier because the methods themselves are wrong. a_an, a_an_uc for instance, is hardly applicable in, say Swedish. We would need others though, to differentiate 'en', 'ett' etc. Hence, all issues and the label provider must be rewritten.
The only way to make it simpler is to retain my previous attempt for this PR and go via provider.a_an in the actual text of the message since that's the place where things must change.
There was a problem hiding this comment.
After discussion we decided to change the label method instead so it takes *args. That fixes the actual problem and leaves the rest alone.
The method MessageData#label has two functions. If called with an argument, it presents the String form for that argument. If called without, it returned a `ModelLabelProvider`. Unfortunately, it decided whether or not it got an argument by testing if it's value was `nil` (the default). This made it impossible to produce a message for the argument `nil`. This commit changes the parameter so that it uses the splat operator and can check the actual number of passed arguments.
thallgren
force-pushed
the
issue/pup-5741/confusing-errors
branch
from
9789174
to
f343cce
Compare
(PUP-5741) Fix confusing errors when comparing values
Prior to this commit, the method MessageData#label had two functions.
If called with an argument, it presented the String form for that
argument. If called without, it returned a
ModelLabelProvider. Thismade it impossible to produce a message for
nil.This commit splits the method in two. One
providermethod that takesno arguments and just returns the
ModuleLabelProviderand one thatalways returns the String representation of its argument. All calls
are adjusted accordingly.