-
Notifications
You must be signed in to change notification settings - Fork 418
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Use toString() to match enum constants #830
Conversation
Picocli sometimes uses `toString()` to get the name of an enum constant, at other times `name()`. These can be different if one overrides `toString()` in the `enum` type declaration, and in that case it is probably because one _wants_ the external name of the enum constant to be different from its source-code name. So that is what options should work in terms of. The `${COMPLETION-CANDITATES}` usage-help interpolation for enum-type arguments already uses `toString()` to display the options. This PR fixes the default type converter such that it works in terms of `toString()` rather than `name()`. It initially will try to match the argument against the raw name using `Enum.valueOf()` because that might use more efficient lookup tables internally (and arguably if someone creates an enum constant whose `toString()` returns the source-level name of a _different_ constant in the same enum, they're setting themselves up for failure anyway).
Codecov Report
@@ Coverage Diff @@
## master #830 +/- ##
============================================
- Coverage 93.25% 93.25% -0.01%
Complexity 385 385
============================================
Files 2 2
Lines 5706 5705 -1
Branches 1485 1485
============================================
- Hits 5321 5320 -1
Misses 185 185
Partials 200 200
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall I think this is a good change.
Can you add unit tests that demonstrate the improvement?
Looking at my old code I realized that it does already use the toString()
value (by calling String.valueOf(enumConstant)
), but only when caseInsensitiveEnumValuesAllowed
is true. This is a bug. Your code does not have that problem, so that is an improvement.
Another bug I just realized (in both the old and the new implementation) is that enumConstant.name()
is never compared case-insensitively, we only do try { return Enum.valueOf(type, value); }
which is essentially case-sensitive matching. Maybe we can fix that while we are at it - I added an inline comment with a suggestion but feel free to do it differently.
Enum<?>[] constants = ((Class<Enum<?>>) type).getEnumConstants(); | ||
String[] names = new String[constants.length]; | ||
for (int i = 0; i < names.length; i++) { names[i] = constants[i].name(); } | ||
for (int i = 0; i < names.length; i++) { names[i] = constants[i].toString(); } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When no match is found, we want to report the enum constant names, not their toString() values: see #592
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, using constants[i].toString()
is better: I realize now that my fix for #592 was wrong. The actual cause for the error reported in #592 was that the parser only looks at the toString()
value of the constants if caseInsensitiveEnumValuesAllowed
is true. If we fix that, it makes sense to print constants[i].toString()
when no match is found.
String thisName = enumConstant.toString(); | ||
if( value.equals(thisName) || insensitive && value.equalsIgnoreCase(thisName) ) { return enumConstant; } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's add a bugfix to also compare enumConstant.name()
case-insensitively:
String str = enumConstant.toString();
String name = enumConstant.name();
if (value.equals(str) || value.equals(name) || insensitive && (value.equalsIgnoreCase(str) || value.equalsIgnoreCase(name))) {
return enumConstant;
}
Also, please keep the whitespace conventions the same as the original. :-) So, if (xxx)
instead of if( xxx )
.
…ring()` as well as their `name()`. Improved error reporting.
Picocli sometimes uses
toString()
to get the name of an enum constant, at other timesname()
. These can be different if one overridestoString()
in theenum
type declaration, and in that case it is probably because one wants the external name of the enum constant to be different from its source-code name. So that is what options should work in terms of.The
${COMPLETION-CANDITATES}
usage-help interpolation for enum-type arguments already usestoString()
to display the options.This PR fixes the default type converter such that it works in terms of
toString()
rather thanname()
. It initially will try to match the argument against the raw name usingEnum.valueOf()
because that might use more efficient lookup tables internally (and arguably if someone creates an enum constant whosetoString()
returns the source-level name of a different constant in the same enum, they're setting themselves up for failure anyway).