Closed petermr closed 4 years ago
Have you tried removing arity = "0..*"
from that option definition?
Note - being run programmatically.
Here's the test:
args = ""
+ "-p " + AMIFixtures.TEST_ZIKA10_DIR
+ " --forcemake"
+ " section"
+ " --sections ALL"
+ " --summary foo"
;
AMI.execute(AMISectionTool.class, args);
System.err.println(""
+ "=======2 runs OK: --summary detects bad arg 'foo' ========\n"
);
args = ""
+ "-p " + AMIFixtures.TEST_ZIKA10_DIR
+ " --forcemake"
+ " section"
+ " --sections ALL"
+ " --summary figure"
;
AMI.execute(AMISectionTool.class, args);
System.err.println(""
+ "=======3 fails: Expected parameter for option '--summary' but found 'figure'========\n"
);
I'll run it with varying Option definitions:
@Option(names = {"--sections"},
arity = "1..*",
description = "sections to extract (uses JATSSectionTagger) %n"
+ "if none, lists Tagger tags%n"
+ "ALL selects all tags in Tagger%n"
+ "AUTO creates hierchical tree based on JATS and heuristics (default)%n,"
)
private List<SectionTag> sectionTagList =
new ArrayList<>(Arrays.asList(new SectionTag[]{SectionTag.AUTO}));
@Option(names = {"--sectiontype"},
arity = "1",
description = "Type of section (XML or HTML) default XML. Probably only used in development")
private SectionType sectionType = SectionType.XML;
@Option(names = {"--summary"},
description = "create summary files for sections (${COMPLETION-CANDIDATES})")
private List<SummaryType> summaryList = new ArrayList<>();
gives
Invalid value for option '--summary' (<summaryList>): expected one of [figure, results, supplementary, table] (case-sensitive) but was 'foo'
Usage: ami section [OPTIONS]
Try 'ami section --help' for more information.
=======2 runs OK: --summary detects bad arg 'foo' ========
Expected parameter for option '--summary' but found 'figure'
Usage: ami section [OPTIONS]
Try 'ami section --help' for more information.
=======3 fails: Expected parameter for option '--summary' but found 'figure'========
@Option(names = {"--sections"},
arity = "1..*",
description = "sections to extract (uses JATSSectionTagger) %n"
+ "if none, lists Tagger tags%n"
+ "ALL selects all tags in Tagger%n"
+ "AUTO creates hierchical tree based on JATS and heuristics (default)%n,"
)
private List<SectionTag> sectionTagList =
new ArrayList<>(Arrays.asList(new SectionTag[]{SectionTag.AUTO}));
@Option(names = {"--sectiontype"},
arity = "1",
description = "Type of section (XML or HTML) default XML. Probably only used in development")
private SectionType sectionType = SectionType.XML;
@Option(names = {"--summary"},
arity = "1..*",
description = "create summary files for sections (${COMPLETION-CANDIDATES})")
private List<SummaryType> summaryList = new ArrayList<>();
gives
Invalid value for option '--summary' at index 0 (<summaryList>): expected one of [figure, results, supplementary, table] (case-sensitive) but was 'foo'
Usage: ami section [OPTIONS]
Try 'ami section --help' for more information.
=======2 runs OK: --summary detects bad arg 'foo' ========
Expected parameter for option '--summary' but found 'figure'
Usage: ami section [OPTIONS]
Try 'ami section --help' for more information.
=======3 fails: Expected parameter for option '--summary' but found 'figure'========
I found the cause:
figure
is also the name of the AMIFigureTool
, which is a sibling command of section
.
Picocli interprets figure
as a new command (since we configured AMI with subcommandsRepeatable = true
); and thus concludes that the --summary
option was missing a mandatory parameter.
We need to disambiguate this somehow.
One idea is to rename either the name for AMIFigureTool
, or the AMISectionTool.SummaryType
enum value.
An alternative is to require end users to quote the value for the --summary
option:
-p src\test\resources\org\contentmine\ami\zika10 --forcemake section --sections ALL --summary "figure"
(The latter means a minor code change in AMI:
Index: src/main/java/org/contentmine/ami/tools/AMI.java
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
--- src/main/java/org/contentmine/ami/tools/AMI.java (date 1593903767568)
+++ src/main/java/org/contentmine/ami/tools/AMI.java (date 1593903767568)
@@ -165,6 +165,7 @@
private static CommandLine createCommandLine() {
CommandLine cmd = new CommandLine(new AMI());
+ cmd.setTrimQuotes(true);
cmd.setParameterExceptionHandler(new ShortErrorMessageHandler());
cmd.setExecutionStrategy(AMI::enhancedLoggingExecutionStrategy);
return cmd;
On Sun, Jul 5, 2020 at 12:06 AM Remko Popma notifications@github.com wrote:
I found the cause: figure is also the name of the AMIFigureTool, which is a sibling command of section.
WELL DONE! I hope it didn't take too long. It's obvious in retrospect.
Picocli interprets figure as a new command (since we configured AMI with subcommandsRepeatable = true); and thus concludes that the --summary option was missing a mandatory parameter.
Understood
We need to disambiguate this somehow. One idea is to rename either the name for AMIFigureTool, or the AMISectionTool.SummaryType enum value.
The single fix is easy - I will rename the enum value.
It's clearly a generic problem and common to any system which uses reserved names. I sometimes us "default" as a name in Java until the compiler refuses.
An alternative is to require end users to quote the value for the --summary option:
-p src\test\resources\org\contentmine\ami\zika10 --forcemake section --sections ALL --summary "figure"
I don't think we need this. It's best for me to choose non-clashing keywords.
The enums are under my control. In principle we could check the values against commands.
Another idea would be syntax for the commands, e.g. case or prefix. I don't like this but it would be safe:
ami -p foo c_search ... c_summary
Are there other scenarios for clashes?
-- Peter Murray-Rust Founder ContentMine.org and Reader Emeritus in Molecular Informatics Dept. Of Chemistry, University of Cambridge, CB2 1EW, UK
I haven’t searched if any other enum values could clash with other command names.
I agree that renaming is probably the easiest and most reliable solution.
The problem is that it only surfaces when the enum value is used. I think I need a way of validating enum values at regular intervals. Does the use of "=" to identify parameter values avoid this?
P.
On Sun, Jul 5, 2020 at 9:18 AM Remko Popma notifications@github.com wrote:
I haven’t searched if any other enum values could clash with other command names.
I agree that renaming is probably the easiest and most reliable solution.
— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/petermr/ami3/issues/48#issuecomment-653856875, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAFTCS7PHWLEBN6PWOWF3SLR2AZM3ANCNFSM4OQL45WA .
-- Peter Murray-Rust Founder ContentMine.org and Reader Emeritus in Molecular Informatics Dept. Of Chemistry, University of Cambridge, CB2 1EW, UK
I haven’t checked but I suspect that putting =
between the option name and the value doesn’t help. That is a good point and potentially something to improve in picocli. (Requires some thought.)
Also, what happens with uncontrolled strings? e.g. if a project has the same name as a reserved (sub) command:
ami -p search search --dictionary search
where I have a project search
(sloppy, easy to do) and a dictionary
called search
(maps to search.xml
).
It's not easy building parsers - I know. I use/d ANTLR which is powerful but wasn't easy to load - but I think it's improved over the years. Is there a formal language specification for picocli? Could one generate a parser from the Commands and Options. Maybe a Parser generator would show the conflicts.
P.
On Sun, Jul 5, 2020 at 10:04 AM Remko Popma notifications@github.com wrote:
I haven’t checked but I suspect that putting = between the option name and the value doesn’t help. That is a good point and potentially something to improve in picocli. (Requires some thought.)
— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/petermr/ami3/issues/48#issuecomment-653861161, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAFTCS64A23AQYGV432ZL5DR2A63FANCNFSM4OQL45WA .
-- Peter Murray-Rust Founder ContentMine.org and Reader Emeritus in Molecular Informatics Dept. Of Chemistry, University of Cambridge, CB2 1EW, UK
what happens with uncontrolled strings? e.g. if a project has the same name as a reserved (sub) command:
ami -p search search --dictionary search
where I have a project
search
(sloppy, easy to do) and a dictionary calledsearch
(maps tosearch.xml
).
Good point. I had not thought of this.
With picocli 4.3.x, the string "search" will be accepted and assigned to the -p
option without any validation.
With picocli 4.4, the above input will fail with error Expected parameter for option '-p' but found 'search'
.
(Background: https://github.com/remkop/picocli/issues/1055)
The picocli 4.4 release notes mention the related case of using option names as option value, for example, what if you have a project that was named --ctree
for some reason. The recommendation is to quote such values. This would also work for your example:
ami -p "search" search --dictionary "search"
However, application authors may want to allow values that match subcommands (or even values that match option names) as option parameters... I am beginning to think the picocli parser is now too strict. I will think about how to address this in a future version of picocli.
It's not easy building parsers - I know. I use/d ANTLR which is powerful but wasn't easy to load - but I think it's improved over the years. Is there a formal language specification for picocli? Could one generate a parser from the Commands and Options. Maybe a Parser generator would show the conflicts.
I am familiar with ANTLR. I don't want to bring in ANTLR as a dependency, but I see your point about formalizing how the picocli parser works. There is no formal language specification, because each picocli application has a unique grammar, defined in their app with picocli annotations. That said, it may be worth formalizing the grammar for the composing elements.
At any rate, I need to think about how to give application flexibility in how to treat ambiguous parameter values.
Similarly for non-enum
values you mentioned in your previous comment. This needs more thought.
Closing since the original problem has been resolved and any follow-up work is in the picocli project.
runs correctly without options. and with
but fails with
see: https://github.com/petermr/ami3/blob/master/src/test/java/org/contentmine/ami/tools/AMISectionToolTest.java