Skip to content
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

Is it possible to create a single value Option typed as byte[] ? #1394

Closed
sbernard31 opened this issue Jul 22, 2021 · 9 comments
Closed

Is it possible to create a single value Option typed as byte[] ? #1394

sbernard31 opened this issue Jul 22, 2021 · 9 comments

Comments

@sbernard31
Copy link
Contributor

sbernard31 commented Jul 22, 2021

I would like to create something like this :

        @Option(required = true,
                names = { "-h", "--hexa" },
                description = { //
                        "an hexadecimal string" },
                converter = HexadecimalConverter.class)
        byte[] hexa;

My converter looks like this :

public class HexadecimalConverter implements ITypeConverter<byte[]> {

    @Override
    public byte[] convert(String value) throws Exception {
        return Hex.decodeHex(value.toCharArray());
    }
}

But I get this kind of error: java.lang.IllegalArgumentException: argument type mismatch.
I think this is because I convert the option in byte[] then picocli want to assign this value to byte[0]. (as it consider any array as a Multiple value option)

I tried to play with arity="1" or type="byte[].class" without success...

Any idea ?

@remkop
Copy link
Owner

remkop commented Jul 23, 2021

Yes picocli considers arrays to be multi-value options, so even when arity=1, users would be able to specify the --hexoption multiple times. This is likely not what you have in mind.

Probably the easiest thing to do is to capture the user-specified token as a string, and convert to a byte array in the application (the run method).

Alternatively, you can have an @Option-annotated setter method that takes a string parameter, convert the string to a byte array in that method and store the value in a byte array field.

@sbernard31
Copy link
Contributor Author

Probably the easiest thing to do is to capture the user-specified token as a string, and convert to a byte array in the application (the run method)

I do not like so much this idea, because I like to have the ITypeConverter which act as validator and converter. This allow to write less code and also have a good error message automatically. (With the model you propose I need to create a ITypeConverter which acts as validator then later in my code do the conversion)

Alternatively, you can have an @Option-annotated setter method that takes a string parameter, convert the string to a byte array in that method and store the value in a byte array field.

I will give a try it, but it feel like a workaround 😞

I would really appreciate that picocli provide an Option parameter which allow to force Array/List/... as single value option. (Not the first time I want to do this kind of thing).

@remkop
Copy link
Owner

remkop commented Jul 27, 2021

Thinking about this more, the @Option-annotated setter method feels a bit better than converting in the application because this way the validation and type conversion logic is in one place and happens during the parsing phase, similar to when using a type converter.

This keeps most of the benefits you mention about the type converter: good error message and centralized testing.
It's just that you would test the setter method instead of the type converter.

Example code:

class MyCommand {
    private byte[] hexa;

    @Option(names = { "-h", "--hexa" }, required = true, description = "a hexadecimal string")
    void setHexa(String value) throws Exception {
        hexa = Hex.decodeHex(value.toCharArray());
    }
}

Picocli currently distinguishes between single-value and multi-value options by their Java type.
If the type is an array, Collection or Map, then the option is a multi-value option, meaning it can be specified multiple times, so mycommand --hexa 1 --hexa 2 --hexa 3 becomes valid input.

I think this makes sense, and is intuitive for programmers trying to build command line applications.

Of course there are exceptions, like your use case. Sometimes byte arrays or character arrays are conceptually one value.
Picocli's job is to make the common case easy, and the uncommon case possible.
I believe the setter method is a fairly clean way to make this uncommon case possible. 😅

@sbernard31
Copy link
Contributor Author

sbernard31 commented Jul 27, 2021

I'm not sure this is so uncommon. At least in my experience this is not the case. 3 applications and 5 option (not only hexa decimal option) where I need to do that.

I use different way to make his work ( setter as you propose or I create a class wrapper)
But clearly there is drawback :

  • less simple do reuse class than a Converter.
  • less simple to test.
  • more code.
  • code less consistent and so less easier to read.

I still feel that adding a new Option argument would be better :

        @Option(required = true,
                names = { "-h", "--hexa" },
                description = { //
                        "an hexadecimal string" },
                converter = HexadecimalConverter.class,
                kind = "single", // or single="true" or forcesinglevalue="true" or something like this ...
        )
        byte[] hexa;

I'm not sure as this is not my case but I also guess that the opposite could be useful, I mean "force an option to be multiple value and store it in something which is not an Array/Map/Collection" ... But maybe this is doable with IParameterConsumer ?
(I'm not sure I didn't play with that currently)

@remkop
Copy link
Owner

remkop commented Jul 27, 2021

Sorry, but I am not convinced. 😅

The amount of code is about the same, it just moves from the separate converter class to the setter method.
Testability is also similar, you can call the setter method from the test.
I honestly don't see much of a problem with the setter method solution.

Most importantly, I like the simplicity of the current picocli design, and I don't feel that introducing a kind = single would be an improvement.

@sbernard31
Copy link
Contributor Author

The amount of code is about the same, it just moves from the separate converter class to the setter method.

Here is just a simplified use case to make it easier to understand.
Sometime the setter is more than one line and a new class need to be create just to encapsulate and make it reusable.

When I see my code with setter and converter, I feel this really less elegant and consistent.

Most importantly, I like the simplicity of the current picocli design, and I don't feel that introducing a kind = single would be an improvement.

I don't know if you are saying that this would make the picocli code more complicated or if you are talking about picocli API ?
For the first one, you probably know that better than me but from the outside It's hard to imagine this would be so impacting.
For the second, for people we don't need it, I can not see how it could hurt. For people like me who need it I guess it would be easier to find than this setter workaround.


I feel that you don't like this idea and so not really open to this change so no need to insist.
I guess I need to live with it even if I'm a bit disappointed by how my code looks like 😞.
Thx anyway to taking time to answer to me.

@sbernard31
Copy link
Contributor Author

#1408 is a drawback of the setter workaround 😞

@remkop
Copy link
Owner

remkop commented Aug 14, 2021

Another idea is to use ByteBuffer as the field type, and have a converter that initializes the ByteBuffer.

@sbernard31
Copy link
Contributor Author

Yep thx to let me know that. 🙏

But this works only for byte[] and I have several other kind of case, like :

  • List<Certificate> truststore; (where param value is a path to a file or a folder or an URI)
  • List<ObjectModel> models; (where param value is a path to a folder)
  • ...

But yes, in a general way, creating a Wrapper like this works :

public class Wrapper<T> {

 private T value;

 public Wrapper(T value) {
    this.value = value;
 }

 public T value(){
     return value;
 }

I just still feel this is less elegant/more verbose than just a param in the @option annotation. :-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants