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 expected for an inherited option to not be matched in the ParsedResult when executing a sub-command? #1159

Closed
metacosm opened this issue Aug 28, 2020 · 10 comments

Comments

@metacosm
Copy link

Context: I have a command with several sub-commands. The top-level command defines some required options which can be either full or abbreviated. When I execute a sub-command, I noticed that not only the inherited options don't appear to be processed as abbreviated (i.e. the parser for the sub-command spec "loses" the fact that abbreviated options are allowed) but also that they don't appear when doing a parsedResult.matchedOption() call… while they do appear when doing a parsedResult.findOption() call.
Is that expected? I could see how it could be expected since, strictly speaking, the option is set on the parent command, not the one that's being executed but I find that confusing: as the option is inherited, I expect it to behave as a regular option on my sub-command…

@remkop
Copy link
Owner

remkop commented Aug 29, 2020

Thank you for raising this! Let me repeat your question to see if I understand it correctly:

  1. inherited options in subcommands don't allow abbreviations, why is this?
  2. inherited options in subcommands don't appear in ParseResult::matchedOption but they do appear in CommandSpec::findOption, why is this?

About question 1, I suspect that when abbreviated options/commands were implemented, we overlooked the possibility that abbreviated options could be combined with inherited options. So this may be a bug. But I am just guessing, I need to create a way to reproduce the issue and step through it to look in more detail.

About question 2, as you point out, the inherited option is actually set on the parent command, but in honesty I did not consider what the expected ParseResult::matchedOption behaviour should be for inherited options.

Interesting question: I tend to use the ParseResult class to make information available that is not available via the annotations. So from that point of view, it would make sense to return the inherited option from ParseResult::matchedOption of the
command where it was actually matched. We could even consider not returning it from ParseResult::matchedOption of the parent command (unless the user actually specified this option on the parent command), however, changing this would be a breaking change... I need to think about this a bit.

@remkop remkop added this to the 4.6 milestone Aug 29, 2020
@remkop
Copy link
Owner

remkop commented Aug 31, 2020

@metacosm, I could not reproduce the first issue you reported (inherited options in subcommands don't allow abbreviations).

Can you provide steps to reproduce the issue?

This test passes:

@Command(name = "Issue1159")
static class Issue1159 {
    @Option(names = "--xxx-yyy", scope = INHERIT)
    int x;

    @Command
    void sub() {
    }
}

@Test
public void testIssue1159() {
    Issue1159 bean = new Issue1159();
    CommandLine cmd = new CommandLine(bean);
    cmd.setAbbreviatedOptionsAllowed(true);

    cmd.parseArgs("--x-y", "123");
    assertEquals(123, bean.x);

    cmd.parseArgs("sub", "--x-y", "345");
    assertEquals(345, bean.x);
}

@remkop
Copy link
Owner

remkop commented Aug 31, 2020

@metacosm, I suspect that the first issue you reported (inherited options in subcommands don't allow abbreviations) was actually caused by a different bug. I created a separate ticket for this: #1162 Abbreviated options are not matched if value attached with '=' separator (like -x=3).

Can you confirm that this is correct? (If I am wrong there is another issue that still needs to be solved...)

After fixing #1162, the below test now passes, so it seems that inherited options can be abbreviated in subcommands. Please let me know if I am overlooking something.

@Test
public void testIssue1159WithEquals() {
    Issue1159 bean = new Issue1159();
    CommandLine cmd = new CommandLine(bean);
    cmd.setAbbreviatedOptionsAllowed(true);
    cmd.parseArgs("--x-y=123");
    assertEquals(123, bean.x);

    cmd.parseArgs("sub", "--x-y=345");
    assertEquals(345, bean.x);
}

@metacosm
Copy link
Author

metacosm commented Sep 1, 2020

I will check hopefully today. However, my options were in the -u foo -p bar form, not -u=x… I will try to come up with a reproducer.

@metacosm
Copy link
Author

metacosm commented Sep 1, 2020

I misunderstood what abbreviated options actually meant… 😅
I thought it meant having a -x version of a --xxx-yyy-zzz option 😄

Here's the use case I wondered about:

  @Command(name = "Issue1159")
    static class Issue1159 {
        @Option(names = {"--xxx-yyy", "-x"}, scope = INHERIT)
        int x;

        @Command
        void sub() {
        }
    }
    
    @Test
    public void testIssue1159() {
        Issue1159 bean = new Issue1159();
        CommandLine cmd = new CommandLine(bean);
        CommandLine.ParseResult parseResult = cmd.parseArgs("sub", "-x", "345");
        Object value = parseResult.matchedOption("-x").getValue(); // this NPEs because matchedOption returns null
        assertEquals(345, bean.x);
    }

I would have expected matchedOption to return something but like I said in the issue description, it can make sense that it would be null.

@remkop
Copy link
Owner

remkop commented Sep 2, 2020

Thanks for that test case.
I have modified it a bit (removed the -x short name for the option), please take a look:

@Command(name = "Issue1159")
static class Issue1159 {
    @Option(names = "--xxx-yyy", scope = INHERIT)
    int x;

    @Command
    void sub() {
    }
}

@Test
public void testIssue1159ParseResult() {
    Issue1159 bean = new Issue1159();
    CommandLine cmd = new CommandLine(bean).setAbbreviatedOptionsAllowed(true);
    ParseResult parseResult = cmd.parseArgs("sub", "--x", "345");
    assertEquals(345, bean.x);

    assertTrue(parseResult.hasSubcommand());
    ParseResult subResult = parseResult.subcommand();

    assertTrue("Matched on the subcommand", subResult.hasMatchedOption("--xxx-yyy")); // works as expected
    Object subValue = subResult.matchedOption("--xxx-yyy").getValue(); // also works correctly
    assertEquals(345, subValue);

    assertFalse("Abbreviated options not supported in ParseResult...",
            subResult.hasMatchedOption("--x")); // this does not work

    assertFalse("Not matched on the parent command",
            parseResult.hasMatchedOption("--xxx-yyy")); // false because the option was not matched on the top-level cmd
}

Note:

  • if we use the ParseResult of the top-level command, hasMatchedOption returns false. This makes sense because the option was not matched on the top level command but on the subcommand.
  • if we use the ParseResult of the subcommand, hasMatchedOption returns true. This also looks fine to me.

Is it possible you were not aware that the ParseResult of the top-level command is a different object than the ParseResult of the subcommand. Was that the case? Perhaps I need to clarify this in the documentation.

The only potential issue I see is that subResult.hasMatchedOption("--x") (using an abbreviated option name in the ParseResult API), it does not work: the ParseResult API only works for the full, unabbreviated, option name.

What do you think?

remkop added a commit that referenced this issue Sep 2, 2020
@metacosm
Copy link
Author

metacosm commented Sep 7, 2020

I think there are several "issues":

  1. I didn't properly understand what "abbreviated" meant. That is solved. :)
  2. My use case actually doesn't have anything to do with abbreviated options… 😅 and is still not clear. The code above doesn't actually reflect it, here's an updated version:
@Command(name = "Issue1159")
    static class Issue1159 {
        @Option(names = {"--foo", "-f"}, scope = INHERIT)
        int x;

        @Command
        void sub() {
        }
    }

    @Test
    public void testIssue1159() {
        Issue1159 bean = new Issue1159();
        CommandLine cmd = new CommandLine(bean);
        final CommandLine.ParseResult sub = cmd.parseArgs("sub", "-f", "345");
        assertEquals("345", sub.commandSpec().findOption('f').getValue().toString());
        assertEquals("345", sub.matchedOption("foo").getValue().toString()); // <= NPE
        assertEquals("345", sub.matchedOption('f').getValue().toString()); // <= NPE
        assertEquals(345, bean.x);
    }

The gist of my issue is whether it is expected that inherited options are not considered matched on sub-commands. As the foo option is inherited and provided, I would have expected it to be matched on the sub-command but maybe that's just not how things are supposed to be (i.e. an option is only matched if it is present and defined as part of the actual command). If that's the case, then I'm not sure I'll ever use matchedOption instead of findOption 😄

Hope this is clearer.

@remkop
Copy link
Owner

remkop commented Sep 8, 2020

Sorry I was unclear.

whether it is expected that inherited options are not considered matched on sub-commands. As the foo option is inherited and provided, I would have expected it to be matched on the sub-command

Your expectation is correct. The foo option is matched on the subcommand. Please note that each command and subcommand (and potential sub-subcommand) has its own parse result. The problem is that the test you showed is looking at the ParseResult for the top-level command, and is not looking at the ParseResult for the subcommand where it was matched.

CommandLine::parseArgs returns the ParseResult for the top-level command.
To get the ParseResult for a subcommand, you need to call ParseResult::subcommand.

With this small modification, your test passes:

@Test
public void testIssue1159() {
    Issue1159 bean = new Issue1159();
    CommandLine cmd = new CommandLine(bean);

    // parse result of the *top-level* command
    CommandLine.ParseResult top = cmd.parseArgs("sub", "-f", "345");
    assertEquals("345", top.commandSpec().findOption('f').getValue().toString()); // unrelated to parse result

    assertNull(top.matchedOption("foo")); // --foo was NOT matched *on the top-level command*
    assertTrue(top.matchedOptions().isEmpty());

    CommandLine.ParseResult sub = top.subcommand(); // get parse result for the subcommand
    assertFalse(sub.matchedOptions().isEmpty()); // some option(s) were matched on the subcommand

    assertEquals("345", sub.matchedOption("foo").getValue().toString()); // --foo WAS matched on the subcommand
    assertEquals("345", sub.matchedOption('f').getValue().toString()); // assertion succeeds
    assertEquals(345, bean.x);
}

@remkop
Copy link
Owner

remkop commented Sep 9, 2020

@metacosm Did this answer your question?

@metacosm
Copy link
Author

metacosm commented Sep 9, 2020

@remkop yes, the distinction between top vs sub parse results wasn't clear to me. Thank you!

@metacosm metacosm closed this as completed Sep 9, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants