Add allowAnyValue #576
Add allowAnyValue #576
Conversation
Added a new option: allowAnyValue
Co-authored-by: DerEchtePilz <[email protected]>
Co-authored-by: DerEchtePilz <[email protected]>
Updated to add exception if non-string mappers are used
| @@ -129,6 +141,9 @@ public ListArgumentBuilderFinished withStringMapper() { | |||
| * @return this list argument builder | |||
| */ | |||
| public ListArgumentBuilderFinished withMapper(Function<T, String> mapper) { | |||
| if (allowAnyValue && !(mapper instanceof StringTooltip)){ | |||
There was a problem hiding this comment.
This instanceof check is not necessary as it will always evaluate to true.
| @@ -129,6 +141,9 @@ public ListArgumentBuilderFinished withStringMapper() { | |||
| * @return this list argument builder | |||
| */ | |||
| public ListArgumentBuilderFinished withMapper(Function<T, String> mapper) { | |||
| if (allowAnyValue && !(mapper instanceof StringTooltip)){ | |||
| throw new IllegalArgumentException("allowAnyValue cannot be used with non-string mappers"); | |||
There was a problem hiding this comment.
I'd say this should be something like:
allowAnyValue can only be set to true if a String mapper is used.
However, I do not know if that even is better so maybe we could wait for other opinions too.
| // should only be used with string mappers so we can ignore the cast warning | ||
| list.add((T) str); |
There was a problem hiding this comment.
| // should only be used with string mappers so we can ignore the cast warning | |
| list.add((T) str); | |
| list.add(values.get(str)); |
As we have the map, I think we should use it instead of resorting to generic casts.
There was a problem hiding this comment.
@DerEchtePilz when I use the mapper it will give me a list of nulls. The only way I've found that works is to directly cast the string to T.
There was a problem hiding this comment.
Well, that sounds like a bug to me that should get fixed. I am currently not available to test anything related to the ListArgument but using the mapper should most definitely not result in null values.
There was a problem hiding this comment.
I'm not sure if it's a bug but maybe just a design flaw. The mapper value only works if the input is in the suggestions list.
That part might not be compatible with what we're trying to accomplish so that's why we made allowAnyValue only valid with strings since we need to cast the string input to T.
| // should only be used with string mappers so we can ignore the cast warning | ||
| list.add((T) str); | ||
| } | ||
| else{ |
There was a problem hiding this comment.
| else{ | |
| else { |
| @@ -57,6 +57,20 @@ $$\downarrow$$ | |||
|
|
|||
| $$\downarrow$$ | |||
|
|
|||
| > ### Allowing any value (Optional) | |||
There was a problem hiding this comment.
In this section I think we should mention that the allowAnyValue option only works for Strings.
Added a new option: allowAnyValue
This allows me to use ListArgumentBuilder for complex tab suggestions and simply passing all input so I can do my own validation.
This allowed me to create this command: https://youtu.be/q03_paw9Vzk