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

parser.nonOptions interaction with withOptionalArg in the case of boolean flags #76

Closed
lbergelson opened this Issue Jan 30, 2015 · 11 comments

Comments

Projects
None yet
2 participants
@lbergelson
Contributor

lbergelson commented Jan 30, 2015

I would like to allow boolean flags to be specified in either of the following ways
--someFlag or --someFlag true
This works well as long as there are no nonOptions

Say I have the following setup.

        OptionParser parser = new OptionParser();
        parser.accepts("flag").withOptionalArg().ofType(Boolean.class);
        parser.nonOptions() ;

        OptionSet parsed = parser.parse("--flag", "1","2");

I would like the following to pass:

        Assert.assertTrue(parsed.has("flag"));
        Assert.assertEquals(parsed.nonOptionArguments(), Arrays.asList("1","2"));

i.e. flag1 is seen, and the following two arguments are interpreted as nonOptionArguments.

What I actually end up with is flag1 is false, and nonOptionArguments contains 3

Is there a way I can achieve the desired behavior?

Ideally:
--flag true 1 2 3 is interpreted as ( --flag true ) ( 1 2 3 )
but
--flag 1 2 3 is interpreted as (--flag) (1 2 3)
It seems possible for the specific case of booleans which could reasonably only accept variants of true or false but doesn't work if the type of flag is String

Alternatively and possibly more coherently.
--flag=true 1 2 3 could be treated differently than --flag true 1 2 3

@pholser

This comment has been minimized.

Show comment
Hide comment
@pholser

pholser Jan 30, 2015

Collaborator

Thanks for your interest!

public class Issue76Test {
    private OptionParser parser;
    private OptionSpec<Boolean> flag;
    private OptionSpec<Integer> nonOptions;

    @Before public void setUp() {
        parser = new OptionParser();
        flag = parser.accepts( "flag" ).withOptionalArg().ofType( Boolean.class );
        nonOptions = parser.nonOptions().ofType( Integer.class );
    }

    @Test public void suppliedExample() {
        OptionSet parsed = parser.parse( "--flag", "1", "2" );

        assertTrue( parsed.has( "flag" ) );
        assertFalse( flag.value( parsed ) );
        assertEquals( asList( 1, 2 ), nonOptions.values( parsed ) );
    }

    @Test public void abuttingValue() {
        OptionSet parsed = parser.parse( "--flag=true", "1", "2" );

        assertTrue( parsed.has( "flag" ) );
        assertTrue( flag.value( parsed ) );
        assertEquals( asList( 1, 2 ), nonOptions.values( parsed ) );
    }

    @Test public void nearValue() {
        OptionSpec<Boolean> flag = parser.accepts( "flag" ).withOptionalArg().ofType( Boolean.class );
        OptionSpec<Integer> nonOptions = parser.nonOptions().ofType( Integer.class );

        OptionSet parsed = parser.parse( "--flag", "true", "1", "2" );

        assertTrue( parsed.has( "flag" ) );
        assertTrue( flag.value( parsed ) );
        assertEquals( asList( 1, 2 ), nonOptions.values( parsed ) );
    }
}

In the latest stuff, suppliedExample doesn't work, but abuttingValue and nearValue do.

When an option is detected, and is known to accept an optional argument, and the following argument does not look like an option, it is consumed and treated as the argument to the option.

Conversion of option arguments to a specified type doesn't actually happen until you ask for the argument's value(s), not at parsing time. So, after parsing, the option set looks like:

--flag: 1
non-options: [2]

When we ask for the value of flag, we see that it's of type boolean, and attempt conversion using Boolean.valueOf(String). The string "1" converts to boolean as false.

One possibility would be to say that if I've recognized an option that accepts an optional arg, and the next arg can't be converted to the appropriate type, then don't try to treat the arg as being attached to the option. If we do this, then probably we should convert booleans so that only "true" and "false" are legitimate values, and every other string fails to convert.

In the meantime, your best bets are:

  1. Make flag not accept an argument. The presence of the option indicates true, its absence false;
  2. Require that flag accept an argument (withRequiredArg), with no optionality. Both --flag=true and --flag true will be treated equivalently.
Collaborator

pholser commented Jan 30, 2015

Thanks for your interest!

public class Issue76Test {
    private OptionParser parser;
    private OptionSpec<Boolean> flag;
    private OptionSpec<Integer> nonOptions;

    @Before public void setUp() {
        parser = new OptionParser();
        flag = parser.accepts( "flag" ).withOptionalArg().ofType( Boolean.class );
        nonOptions = parser.nonOptions().ofType( Integer.class );
    }

    @Test public void suppliedExample() {
        OptionSet parsed = parser.parse( "--flag", "1", "2" );

        assertTrue( parsed.has( "flag" ) );
        assertFalse( flag.value( parsed ) );
        assertEquals( asList( 1, 2 ), nonOptions.values( parsed ) );
    }

    @Test public void abuttingValue() {
        OptionSet parsed = parser.parse( "--flag=true", "1", "2" );

        assertTrue( parsed.has( "flag" ) );
        assertTrue( flag.value( parsed ) );
        assertEquals( asList( 1, 2 ), nonOptions.values( parsed ) );
    }

    @Test public void nearValue() {
        OptionSpec<Boolean> flag = parser.accepts( "flag" ).withOptionalArg().ofType( Boolean.class );
        OptionSpec<Integer> nonOptions = parser.nonOptions().ofType( Integer.class );

        OptionSet parsed = parser.parse( "--flag", "true", "1", "2" );

        assertTrue( parsed.has( "flag" ) );
        assertTrue( flag.value( parsed ) );
        assertEquals( asList( 1, 2 ), nonOptions.values( parsed ) );
    }
}

In the latest stuff, suppliedExample doesn't work, but abuttingValue and nearValue do.

When an option is detected, and is known to accept an optional argument, and the following argument does not look like an option, it is consumed and treated as the argument to the option.

Conversion of option arguments to a specified type doesn't actually happen until you ask for the argument's value(s), not at parsing time. So, after parsing, the option set looks like:

--flag: 1
non-options: [2]

When we ask for the value of flag, we see that it's of type boolean, and attempt conversion using Boolean.valueOf(String). The string "1" converts to boolean as false.

One possibility would be to say that if I've recognized an option that accepts an optional arg, and the next arg can't be converted to the appropriate type, then don't try to treat the arg as being attached to the option. If we do this, then probably we should convert booleans so that only "true" and "false" are legitimate values, and every other string fails to convert.

In the meantime, your best bets are:

  1. Make flag not accept an argument. The presence of the option indicates true, its absence false;
  2. Require that flag accept an argument (withRequiredArg), with no optionality. Both --flag=true and --flag true will be treated equivalently.
@lbergelson

This comment has been minimized.

Show comment
Hide comment
@lbergelson

lbergelson Jan 30, 2015

Contributor

Thanks for the response. You are a hero of rapid response times.

Both options 1 and 2 make total sense. Unfortunately my project is absorbing two existing projects with legacy users. Half of them want to specify flags like option 1 and half want option 2. So I've been trying to accept either style.
The third option I guess is to just require that they specify an end of options delimiter if using flag and nonOptions but it seems like really unexpected behavior.

I think the possibility you mention of checking that you can do the conversion before consuming the next argument is a good idea (it's kind of what I was expecting to happen). The fact that anything converts to a boolean is a bit of a pain but could be special cased.

The other option I see is to check the remaining arguments. If none of them looksLikeAnOption then assume that they are all nonOptions.

Contributor

lbergelson commented Jan 30, 2015

Thanks for the response. You are a hero of rapid response times.

Both options 1 and 2 make total sense. Unfortunately my project is absorbing two existing projects with legacy users. Half of them want to specify flags like option 1 and half want option 2. So I've been trying to accept either style.
The third option I guess is to just require that they specify an end of options delimiter if using flag and nonOptions but it seems like really unexpected behavior.

I think the possibility you mention of checking that you can do the conversion before consuming the next argument is a good idea (it's kind of what I was expecting to happen). The fact that anything converts to a boolean is a bit of a pain but could be special cased.

The other option I see is to check the remaining arguments. If none of them looksLikeAnOption then assume that they are all nonOptions.

@pholser

This comment has been minimized.

Show comment
Hide comment
@pholser

pholser Jan 30, 2015

Collaborator

@lbergelson Excellent - I think I will proceed as follows:

  1. if I've recognized an option that accepts an optional arg, and the next arg can't be converted to the appropriate type, then don't try to treat the arg as being attached to the option.

  2. Expose a StrictJavaBooleanConverter that accepts only "true" and "false". You can then say:

parser.accepts( "flag" )
    .withOptionalArg()
    .withValuesConvertedBy(new StrictJavaBooleanConverter());

How does this sound?

Collaborator

pholser commented Jan 30, 2015

@lbergelson Excellent - I think I will proceed as follows:

  1. if I've recognized an option that accepts an optional arg, and the next arg can't be converted to the appropriate type, then don't try to treat the arg as being attached to the option.

  2. Expose a StrictJavaBooleanConverter that accepts only "true" and "false". You can then say:

parser.accepts( "flag" )
    .withOptionalArg()
    .withValuesConvertedBy(new StrictJavaBooleanConverter());

How does this sound?

@lbergelson

This comment has been minimized.

Show comment
Hide comment
@lbergelson

lbergelson Jan 30, 2015

Contributor

That sounds like it would exactly solve my problem. I might relax StrictJavaBooleanConverter to be slightly more accepting. I'd accept in case insensitive way true/t/false/f. If you don't like that though I can always just write my own converter.

Contributor

lbergelson commented Jan 30, 2015

That sounds like it would exactly solve my problem. I might relax StrictJavaBooleanConverter to be slightly more accepting. I'd accept in case insensitive way true/t/false/f. If you don't like that though I can always just write my own converter.

pholser added a commit that referenced this issue Jan 30, 2015

For #76, rethink typed optional args of options.
With this change, when we encounter an option that accepts an optional
argument, then if the next argument does not "look like" an option, and
the argument can be converted to the desired type, then we'll treat the
arg as the argument of the option. If the next argument "looks like" an
option, but the option's argument is a number type, then we'll treat the
argument as the numeric value of the option. (This allows for things like
--factor -2.)

If neither of the above scenarios applies, then the option is considered
not to have an argument.
@pholser

This comment has been minimized.

Show comment
Hide comment
@pholser

pholser Jan 30, 2015

Collaborator

@lbergelson Latest 4.9 snapshot at oss.sonatype.org should have 1) above. I elected not to add a StrictJavaBooleanConverter -- hopefully rolling your own won't be too much trouble.

Thanks again! Interesting case!

Collaborator

pholser commented Jan 30, 2015

@lbergelson Latest 4.9 snapshot at oss.sonatype.org should have 1) above. I elected not to add a StrictJavaBooleanConverter -- hopefully rolling your own won't be too much trouble.

Thanks again! Interesting case!

@lbergelson

This comment has been minimized.

Show comment
Hide comment
@lbergelson

lbergelson Jan 30, 2015

Contributor

Thank you! Shouldn't be a problem to make my own.

On Fri, Jan 30, 2015 at 5:06 PM, Paul Holser notifications@github.com
wrote:

@lbergelson https://github.com/lbergelson Latest 4.9 snapshot at
oss.sonatype.org should have 1) above. I elected not to add a
StrictJavaBooleanConverter -- hopefully rolling your own won't be too
much trouble.

Thanks again! Interesting case!


Reply to this email directly or view it on GitHub
#76 (comment).

Contributor

lbergelson commented Jan 30, 2015

Thank you! Shouldn't be a problem to make my own.

On Fri, Jan 30, 2015 at 5:06 PM, Paul Holser notifications@github.com
wrote:

@lbergelson https://github.com/lbergelson Latest 4.9 snapshot at
oss.sonatype.org should have 1) above. I elected not to add a
StrictJavaBooleanConverter -- hopefully rolling your own won't be too
much trouble.

Thanks again! Interesting case!


Reply to this email directly or view it on GitHub
#76 (comment).

@pholser

This comment has been minimized.

Show comment
Hide comment
@pholser

pholser Feb 1, 2015

Collaborator

@lbergelson Great - let me know how this goes. If all's well, can close the issue.

Collaborator

pholser commented Feb 1, 2015

@lbergelson Great - let me know how this goes. If all's well, can close the issue.

@lbergelson

This comment has been minimized.

Show comment
Hide comment
@lbergelson

lbergelson Feb 3, 2015

Contributor

Sorry for the delay! We had some distracting snow here.

It's working exactly as expected. Thanks again.

Is a numbered release in maven central likely any time soon? It's a bit scary building against a changing target.

Contributor

lbergelson commented Feb 3, 2015

Sorry for the delay! We had some distracting snow here.

It's working exactly as expected. Thanks again.

Is a numbered release in maven central likely any time soon? It's a bit scary building against a changing target.

@pholser

This comment has been minimized.

Show comment
Hide comment
@pholser

pholser Feb 3, 2015

Collaborator

Planning on cutting a 4.9 beta release this week.

Collaborator

pholser commented Feb 3, 2015

Planning on cutting a 4.9 beta release this week.

@pholser

This comment has been minimized.

Show comment
Hide comment
@pholser

pholser Feb 9, 2015

Collaborator

@lbergelson 4.9 beta 1 is at Maven central.

Collaborator

pholser commented Feb 9, 2015

@lbergelson 4.9 beta 1 is at Maven central.

@lbergelson

This comment has been minimized.

Show comment
Hide comment
@lbergelson

lbergelson Feb 9, 2015

Contributor

Thanks.
On Feb 9, 2015 9:53 AM, "Paul Holser" notifications@github.com wrote:

@lbergelson https://github.com/lbergelson 4.9 beta 1 is at Maven
central.


Reply to this email directly or view it on GitHub
#76 (comment).

Contributor

lbergelson commented Feb 9, 2015

Thanks.
On Feb 9, 2015 9:53 AM, "Paul Holser" notifications@github.com wrote:

@lbergelson https://github.com/lbergelson 4.9 beta 1 is at Maven
central.


Reply to this email directly or view it on GitHub
#76 (comment).

@lbergelson lbergelson closed this Feb 9, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment