diff options
5 files changed, 244 insertions, 93 deletions
diff --git a/src/main/java/com/google/devtools/common/options/OptionInstanceOrigin.java b/src/main/java/com/google/devtools/common/options/OptionInstanceOrigin.java new file mode 100644 index 0000000000..584e75b9fc --- /dev/null +++ b/src/main/java/com/google/devtools/common/options/OptionInstanceOrigin.java @@ -0,0 +1,57 @@ +// Copyright 2017 The Bazel Authors. All rights reserved. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. +package com.google.devtools.common.options; + +import javax.annotation.Nullable; + +/** + * Contains metadata describing the origin of an option. This includes its priority, a message about + * where it came from, and whether it was set explicitly or expanded/implied by other flags. + */ +public class OptionInstanceOrigin { + private final OptionPriority priority; + @Nullable private final String source; + @Nullable private final OptionDefinition implicitDependent; + @Nullable private final OptionDefinition expandedFrom; + + public OptionInstanceOrigin( + OptionPriority priority, + String source, + OptionDefinition implicitDependent, + OptionDefinition expandedFrom) { + this.priority = priority; + this.source = source; + this.implicitDependent = implicitDependent; + this.expandedFrom = expandedFrom; + } + + public OptionPriority getPriority() { + return priority; + } + + @Nullable + public String getSource() { + return source; + } + + @Nullable + public OptionDefinition getImplicitDependent() { + return implicitDependent; + } + + @Nullable + public OptionDefinition getExpandedFrom() { + return expandedFrom; + } +} diff --git a/src/main/java/com/google/devtools/common/options/OptionValueDescription.java b/src/main/java/com/google/devtools/common/options/OptionValueDescription.java index 8222aee12b..0d81d49a84 100644 --- a/src/main/java/com/google/devtools/common/options/OptionValueDescription.java +++ b/src/main/java/com/google/devtools/common/options/OptionValueDescription.java @@ -26,9 +26,7 @@ import java.util.stream.Collectors; /** * The value of an option. * - * <p>This takes care of tracking the final value as multiple instances of an option are parsed. It - * also tracks additional metadata describing its priority, source, whether it was set via an - * implicit dependency, and if so, by which other option. + * <p>This takes care of tracking the final value as multiple instances of an option are parsed. */ public abstract class OptionValueDescription { @@ -48,12 +46,8 @@ public abstract class OptionValueDescription { /** Returns the source(s) of this option, if there were multiple, duplicates are removed. */ public abstract String getSourceString(); - // TODO(b/65540004) implicitDependant and expandedFrom are artifacts of an option instance, and - // should be in ParsedOptionDescription. abstract void addOptionInstance( ParsedOptionDescription parsedOption, - OptionDefinition implicitDependant, - OptionDefinition expandedFrom, List<String> warnings) throws OptionsParsingException; @@ -105,8 +99,6 @@ public abstract class OptionValueDescription { @Override void addOptionInstance( ParsedOptionDescription parsedOption, - OptionDefinition implicitDependant, - OptionDefinition expandedFrom, List<String> warnings) { throw new IllegalStateException( "Cannot add values to the default option value. Create a modifiable " @@ -121,8 +113,6 @@ public abstract class OptionValueDescription { static class SingleOptionValueDescription extends OptionValueDescription { private ParsedOptionDescription effectiveOptionInstance; private Object effectiveValue; - private OptionDefinition optionThatDependsOnEffectiveValue; - private OptionDefinition optionThatExpandedToEffectiveValue; private SingleOptionValueDescription(OptionDefinition optionDefinition) { super(optionDefinition); @@ -134,8 +124,6 @@ public abstract class OptionValueDescription { } effectiveOptionInstance = null; effectiveValue = null; - optionThatDependsOnEffectiveValue = null; - optionThatExpandedToEffectiveValue = null; } @Override @@ -152,15 +140,11 @@ public abstract class OptionValueDescription { @Override void addOptionInstance( ParsedOptionDescription parsedOption, - OptionDefinition implicitDependant, - OptionDefinition expandedFrom, List<String> warnings) throws OptionsParsingException { // This might be the first value, in that case, just store it! if (effectiveOptionInstance == null) { effectiveOptionInstance = parsedOption; - optionThatDependsOnEffectiveValue = implicitDependant; - optionThatExpandedToEffectiveValue = expandedFrom; effectiveValue = effectiveOptionInstance.getConvertedValue(); return; } @@ -168,23 +152,31 @@ public abstract class OptionValueDescription { // If there was another value, check whether the new one will override it, and if so, // log warnings describing the change. if (parsedOption.getPriority().compareTo(effectiveOptionInstance.getPriority()) >= 0) { + // Identify the option that might have led to the current and new value of this option. + OptionDefinition implicitDependent = parsedOption.getImplicitDependent(); + OptionDefinition expandedFrom = parsedOption.getExpandedFrom(); + OptionDefinition optionThatDependsOnEffectiveValue = + effectiveOptionInstance.getImplicitDependent(); + OptionDefinition optionThatExpandedToEffectiveValue = + effectiveOptionInstance.getExpandedFrom(); + // Output warnings: - if ((implicitDependant != null) && (optionThatDependsOnEffectiveValue != null)) { - if (!implicitDependant.equals(optionThatDependsOnEffectiveValue)) { + if ((implicitDependent != null) && (optionThatDependsOnEffectiveValue != null)) { + if (!implicitDependent.equals(optionThatDependsOnEffectiveValue)) { warnings.add( String.format( "Option '%s' is implicitly defined by both option '%s' and option '%s'", optionDefinition.getOptionName(), optionThatDependsOnEffectiveValue.getOptionName(), - implicitDependant.getOptionName())); + implicitDependent.getOptionName())); } - } else if ((implicitDependant != null) + } else if ((implicitDependent != null) && parsedOption.getPriority().equals(effectiveOptionInstance.getPriority())) { warnings.add( String.format( "Option '%s' is implicitly defined by option '%s'; the implicitly set value " + "overrides the previous one", - optionDefinition.getOptionName(), implicitDependant.getOptionName())); + optionDefinition.getOptionName(), implicitDependent.getOptionName())); } else if (optionThatDependsOnEffectiveValue != null) { warnings.add( String.format( @@ -211,8 +203,6 @@ public abstract class OptionValueDescription { // Record the new value: effectiveOptionInstance = parsedOption; - optionThatDependsOnEffectiveValue = implicitDependant; - optionThatExpandedToEffectiveValue = expandedFrom; effectiveValue = parsedOption.getConvertedValue(); } else { // The new value does not override the old value, as it has lower priority. @@ -275,17 +265,17 @@ public abstract class OptionValueDescription { @Override void addOptionInstance( ParsedOptionDescription parsedOption, - OptionDefinition implicitDependant, - OptionDefinition expandedFrom, List<String> warnings) throws OptionsParsingException { // For repeatable options, we allow flags that take both single values and multiple values, // potentially collapsing them down. Object convertedValue = parsedOption.getConvertedValue(); + OptionPriority priority = parsedOption.getPriority(); + parsedOptions.put(priority, parsedOption); if (convertedValue instanceof List<?>) { - optionValues.putAll(parsedOption.getPriority(), (List<?>) convertedValue); + optionValues.putAll(priority, (List<?>) convertedValue); } else { - optionValues.put(parsedOption.getPriority(), convertedValue); + optionValues.put(priority, convertedValue); } } } @@ -318,8 +308,6 @@ public abstract class OptionValueDescription { @Override void addOptionInstance( ParsedOptionDescription parsedOption, - OptionDefinition implicitDependant, - OptionDefinition expandedFrom, List<String> warnings) { // TODO(b/65540004) Deal with expansion options here instead of in parse(), and track their // link to the options they expanded to to. @@ -341,13 +329,11 @@ public abstract class OptionValueDescription { @Override void addOptionInstance( ParsedOptionDescription parsedOption, - OptionDefinition implicitDependant, - OptionDefinition expandedFrom, List<String> warnings) throws OptionsParsingException { // This is a valued flag, its value is handled the same way as a normal // SingleOptionValueDescription. - super.addOptionInstance(parsedOption, implicitDependant, expandedFrom, warnings); + super.addOptionInstance(parsedOption, warnings); // Now deal with the implicit requirements. // TODO(b/65540004) Deal with options with implicit requirements here instead of in parse(), @@ -375,8 +361,6 @@ public abstract class OptionValueDescription { @Override void addOptionInstance( ParsedOptionDescription parsedOption, - OptionDefinition implicitDependant, - OptionDefinition expandedFrom, List<String> warnings) throws OptionsParsingException { // TODO(b/65540004) Deal with options with implicit requirements here instead of in parse(), diff --git a/src/main/java/com/google/devtools/common/options/OptionsParserImpl.java b/src/main/java/com/google/devtools/common/options/OptionsParserImpl.java index a7745f6fe6..176d51e66a 100644 --- a/src/main/java/com/google/devtools/common/options/OptionsParserImpl.java +++ b/src/main/java/com/google/devtools/common/options/OptionsParserImpl.java @@ -223,17 +223,17 @@ class OptionsParserImpl { return new OptionDescription( optionDefinition, optionsData.getExpansionDataForField(optionDefinition), - getImplicitDependantDescriptions( + getImplicitDependentDescriptions( ImmutableList.copyOf(optionDefinition.getImplicitRequirements()), optionDefinition, priority, source)); } - /** @return A list of the descriptions corresponding to the implicit dependant flags passed in. */ - private ImmutableList<ParsedOptionDescription> getImplicitDependantDescriptions( + /** @return A list of the descriptions corresponding to the implicit dependent flags passed in. */ + private ImmutableList<ParsedOptionDescription> getImplicitDependentDescriptions( ImmutableList<String> options, - OptionDefinition implicitDependant, + OptionDefinition implicitDependent, OptionPriority priority, String source) throws OptionsParsingException { @@ -244,12 +244,17 @@ class OptionsParserImpl { o -> String.format( "implicitely required for option %s (source: %s)", - implicitDependant.getOptionName(), source); + implicitDependent.getOptionName(), source); while (optionsIterator.hasNext()) { String unparsedFlagExpression = optionsIterator.next(); ParsedOptionDescription parsedOption = identifyOptionAndPossibleArgument( - unparsedFlagExpression, optionsIterator, priority, sourceFunction, false); + unparsedFlagExpression, + optionsIterator, + priority, + sourceFunction, + implicitDependent, + null); builder.add(parsedOption); } return builder.build(); @@ -257,9 +262,7 @@ class OptionsParserImpl { /** * @return A list of the descriptions corresponding to options expanded from the flag for the - * given value. These descriptions are are divorced from the command line - there is no - * correct priority or source for these, as they are not actually set values. The value itself - * is also a string, no conversion has taken place. + * given value. The value itself is a string, no conversion has taken place. */ ImmutableList<ParsedOptionDescription> getExpansionOptionValueDescriptions( OptionDefinition expansionFlag, @@ -277,7 +280,12 @@ class OptionsParserImpl { String unparsedFlagExpression = optionsIterator.next(); ParsedOptionDescription parsedOption = identifyOptionAndPossibleArgument( - unparsedFlagExpression, optionsIterator, priority, sourceFunction, false); + unparsedFlagExpression, + optionsIterator, + priority, + sourceFunction, + null, + expansionFlag); builder.add(parsedOption); } return builder.build(); @@ -317,7 +325,6 @@ class OptionsParserImpl { OptionDefinition expandedFrom, List<String> args) throws OptionsParsingException { - boolean isExplicit = expandedFrom == null && implicitDependent == null; List<String> unparsedArgs = new ArrayList<>(); LinkedHashMap<OptionDefinition, List<String>> implicitRequirements = new LinkedHashMap<>(); @@ -337,7 +344,7 @@ class OptionsParserImpl { ParsedOptionDescription parsedOption = identifyOptionAndPossibleArgument( - arg, argsIterator, priority, sourceFunction, isExplicit); + arg, argsIterator, priority, sourceFunction, implicitDependent, expandedFrom); OptionDefinition optionDefinition = parsedOption.getOptionDefinition(); // All options can be deprecated; check and warn before doing any option-type specific work. maybeAddDeprecationWarning(optionDefinition); @@ -347,7 +354,7 @@ class OptionsParserImpl { OptionValueDescription entry = optionValues.computeIfAbsent( optionDefinition, OptionValueDescription::createOptionValueDescription); - entry.addOptionInstance(parsedOption, implicitDependent, expandedFrom, warnings); + entry.addOptionInstance(parsedOption, warnings); @Nullable String unconvertedValue = parsedOption.getUnconvertedValue(); if (optionDefinition.isWrapperOption()) { @@ -404,11 +411,13 @@ class OptionsParserImpl { ImmutableList<String> expansion = optionsData.getEvaluatedExpansion(optionDefinition, unconvertedValue); + String sourceFunctionApplication = sourceFunction.apply(optionDefinition); String sourceMessage = - "expanded from option --" - + optionDefinition.getOptionName() - + " from " - + sourceFunction.apply(optionDefinition); + (sourceFunctionApplication == null) + ? String.format("expanded from option --%s", optionDefinition.getOptionName()) + : String.format( + "expanded from option --%s from %s", + optionDefinition.getOptionName(), sourceFunctionApplication); Function<OptionDefinition, String> expansionSourceFunction = o -> sourceMessage; List<String> unparsed = parse(priority, expansionSourceFunction, null, optionDefinition, expansion); @@ -434,11 +443,15 @@ class OptionsParserImpl { // TODO(bazel-team): this should happen when the option is encountered. if (!implicitRequirements.isEmpty()) { for (Map.Entry<OptionDefinition, List<String>> entry : implicitRequirements.entrySet()) { + OptionDefinition optionDefinition = entry.getKey(); + String sourceFunctionApplication = sourceFunction.apply(optionDefinition); String sourceMessage = - "implicit requirement of option --" - + entry.getKey() - + " from " - + sourceFunction.apply(entry.getKey()); + (sourceFunctionApplication == null) + ? String.format( + "implicit requirement of option --%s", optionDefinition.getOptionName()) + : String.format( + "implicit requirement of option --%s from %s", + optionDefinition.getOptionName(), sourceFunctionApplication); Function<OptionDefinition, String> requirementSourceFunction = o -> sourceMessage; List<String> unparsed = parse(priority, requirementSourceFunction, entry.getKey(), null, @@ -467,7 +480,8 @@ class OptionsParserImpl { Iterator<String> nextArgs, OptionPriority priority, Function<OptionDefinition, String> sourceFunction, - boolean explicit) + OptionDefinition implicitDependent, + OptionDefinition expandedFrom) throws OptionsParsingException { // Store the way this option was parsed on the command line. @@ -548,9 +562,8 @@ class OptionsParserImpl { optionDefinition, commandLineForm.toString(), unconvertedValue, - priority, - sourceFunction.apply(optionDefinition), - explicit); + new OptionInstanceOrigin( + priority, sourceFunction.apply(optionDefinition), implicitDependent, expandedFrom)); } /** diff --git a/src/main/java/com/google/devtools/common/options/ParsedOptionDescription.java b/src/main/java/com/google/devtools/common/options/ParsedOptionDescription.java index 036ac4188b..0910579541 100644 --- a/src/main/java/com/google/devtools/common/options/ParsedOptionDescription.java +++ b/src/main/java/com/google/devtools/common/options/ParsedOptionDescription.java @@ -18,39 +18,27 @@ import com.google.common.collect.ImmutableList; import javax.annotation.Nullable; /** - * The value of an option with additional metadata describing its origin. + * The representation of a parsed option instance. * - * <p>This class represents an option as the parser received it, which is distinct from the final - * value of an option, as these values may be overridden or combined in some way. - * - * <p>The origin includes the form it had when parsed, its priority, a message about where it came - * from, and whether it was set explicitly or expanded/implied by other flags. + * <p>An option instance is distinct from the final value of an option, as multiple instances + * provide values may be overridden or combined in some way. */ public final class ParsedOptionDescription { + private final OptionDefinition optionDefinition; private final String commandLineForm; @Nullable private final String unconvertedValue; - private final OptionPriority priority; - @Nullable private final String source; - - // Whether this flag was explicitly given, as opposed to having been added by an expansion flag - // or an implicit dependency. Notice that this does NOT mean it was explicitly given by the - // user, for that to be true, it needs the right combination of explicit & priority. - private final boolean explicit; + private final OptionInstanceOrigin origin; public ParsedOptionDescription( OptionDefinition optionDefinition, String commandLineForm, @Nullable String unconvertedValue, - OptionPriority priority, - @Nullable String source, - boolean explicit) { + OptionInstanceOrigin origin) { this.optionDefinition = optionDefinition; this.commandLineForm = commandLineForm; this.unconvertedValue = unconvertedValue; - this.priority = priority; - this.source = source; - this.explicit = explicit; + this.origin = origin; } public OptionDefinition getOptionDefinition() { @@ -87,15 +75,23 @@ public final class ParsedOptionDescription { } OptionPriority getPriority() { - return priority; + return origin.getPriority(); } public String getSource() { - return source; + return origin.getSource(); + } + + OptionDefinition getImplicitDependent() { + return origin.getImplicitDependent(); + } + + OptionDefinition getExpandedFrom() { + return origin.getExpandedFrom(); } public boolean isExplicit() { - return explicit; + return origin.getExpandedFrom() == null && origin.getImplicitDependent() == null; } public Object getConvertedValue() throws OptionsParsingException { @@ -114,9 +110,9 @@ public final class ParsedOptionDescription { StringBuilder result = new StringBuilder(); result.append("option '").append(optionDefinition.getOptionName()).append("' "); result.append("set to '").append(unconvertedValue).append("' "); - result.append("with priority ").append(priority); - if (source != null) { - result.append(" and source '").append(source).append("'"); + result.append("with priority ").append(origin.getPriority()); + if (origin.getSource() != null) { + result.append(" and source '").append(origin.getSource()).append("'"); } return result.toString(); } diff --git a/src/test/java/com/google/devtools/common/options/OptionsParserTest.java b/src/test/java/com/google/devtools/common/options/OptionsParserTest.java index 56e1b969c7..77bcc36412 100644 --- a/src/test/java/com/google/devtools/common/options/OptionsParserTest.java +++ b/src/test/java/com/google/devtools/common/options/OptionsParserTest.java @@ -22,6 +22,7 @@ import static org.junit.Assert.fail; import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; import com.google.devtools.common.options.Converters.CommaSeparatedOptionListConverter; +import com.google.devtools.common.options.OptionValueDescription.RepeatableOptionValueDescription; import com.google.devtools.common.options.OptionValueDescription.SingleOptionValueDescription; import java.io.ByteArrayInputStream; import java.io.ByteArrayOutputStream; @@ -1126,6 +1127,25 @@ public class OptionsParserTest { } @Test + public void testExpansionOriginIsPropagatedToOption() throws OptionsParsingException { + OptionsParser parser = OptionsParser.newOptionsParser(ExpansionOptions.class); + parser.parse(OptionPriority.COMMAND_LINE, null, Arrays.asList("--expands")); + OptionValueDescription expansionDescription = parser.getOptionValueDescription("expands"); + assertThat(expansionDescription).isNotNull(); + + // In order to have access to the ParsedOptionDescription tracked by the value of 'underlying' + // we have to know that this option is a "single valued" option. + SingleOptionValueDescription underlyingDescription = + (SingleOptionValueDescription) parser.getOptionValueDescription("underlying"); + assertThat(underlyingDescription).isNotNull(); + assertThat(underlyingDescription.getSourceString()).matches("expanded from option --expands"); + assertThat(underlyingDescription.getEffectiveOptionInstance()).isNotNull(); + assertThat(underlyingDescription.getEffectiveOptionInstance().getExpandedFrom()) + .isSameAs(expansionDescription.getOptionDefinition()); + assertThat(underlyingDescription.getEffectiveOptionInstance().getImplicitDependent()).isNull(); + } + + @Test public void overrideExplicitWithExpansion() throws Exception { OptionsParser parser = OptionsParser.newOptionsParser(ExpansionOptions.class); parser.parse( @@ -1199,6 +1219,8 @@ public class OptionsParserTest { ParsedOptionDescription singleOptionInstance = singleOptionResult.getEffectiveOptionInstance(); assertThat(singleOptionInstance.getPriority()).isEqualTo(OptionPriority.COMMAND_LINE); assertThat(singleOptionInstance.getOptionDefinition().isExpansionOption()).isFalse(); + assertThat(singleOptionInstance.getImplicitDependent()).isNull(); + assertThat(singleOptionInstance.getExpandedFrom()).isNull(); } public static class ImplicitDependencyWarningOptions extends OptionsBase { @@ -1269,6 +1291,25 @@ public class OptionsParserTest { + "option 'first' and option 'third'"); } + @Test + public void tesDependentOriginIsPropagatedToOption() throws OptionsParsingException { + OptionsParser parser = OptionsParser.newOptionsParser(ImplicitDependencyWarningOptions.class); + parser.parse(OptionPriority.COMMAND_LINE, null, Arrays.asList("--first=first")); + OptionValueDescription originalOption = parser.getOptionValueDescription("first"); + assertThat(originalOption).isNotNull(); + + // In order to have access to the ParsedOptionDescription tracked by the value of 'underlying' + // we have to know that this option is a "single valued" option. + SingleOptionValueDescription requiredOption = + (SingleOptionValueDescription) parser.getOptionValueDescription("second"); + assertThat(requiredOption).isNotNull(); + assertThat(requiredOption.getSourceString()).matches("implicit requirement of option --first"); + assertThat(requiredOption.getEffectiveOptionInstance()).isNotNull(); + assertThat(requiredOption.getEffectiveOptionInstance().getExpandedFrom()).isNull(); + assertThat(requiredOption.getEffectiveOptionInstance().getImplicitDependent()) + .isSameAs(originalOption.getOptionDefinition()); + } + public static class WarningOptions extends OptionsBase { @Deprecated @Option( @@ -1609,8 +1650,6 @@ public class OptionsParserTest { assertOptionValue("echo", "echoDefaultValue", map.get("echo")); } - // Regression tests for bug: - // "--option from blazerc unexpectedly overrides --option from command line" public static class ListExample extends OptionsBase { @Option( name = "alpha", @@ -1623,12 +1662,42 @@ public class OptionsParserTest { public List<String> alpha; } + // Regression tests for bug: + // "--option from blazerc unexpectedly overrides --option from command line" @Test public void overrideListOptions() throws Exception { OptionsParser parser = OptionsParser.newOptionsParser(ListExample.class); - parser.parse(OptionPriority.COMMAND_LINE, "a", Arrays.asList("--alpha=two")); - parser.parse(OptionPriority.RC_FILE, "b", Arrays.asList("--alpha=one")); - assertThat(parser.getOptions(ListExample.class).alpha).isEqualTo(Arrays.asList("one", "two")); + parser.parse(OptionPriority.COMMAND_LINE, "command line source", Arrays.asList("--alpha=cli")); + parser.parse( + OptionPriority.RC_FILE, "rc file origin", Arrays.asList("--alpha=rc1", "--alpha=rc2")); + assertThat(parser.getOptions(ListExample.class).alpha) + .isEqualTo(Arrays.asList("rc1", "rc2", "cli")); + } + + @Test + public void listOptionsHaveCorrectPriorities() throws Exception { + OptionsParser parser = OptionsParser.newOptionsParser(ListExample.class); + parser.parse(OptionPriority.COMMAND_LINE, "command line source", Arrays.asList("--alpha=cli")); + parser.parse( + OptionPriority.RC_FILE, "rc file origin", Arrays.asList("--alpha=rc1", "--alpha=rc2")); + + OptionValueDescription alphaValue = parser.getOptionValueDescription("alpha"); + assertThat(alphaValue).isInstanceOf(RepeatableOptionValueDescription.class); + + RepeatableOptionValueDescription alpha = (RepeatableOptionValueDescription) alphaValue; + assertThat(alpha.parsedOptions).containsKey(OptionPriority.RC_FILE); + assertThat(alpha.parsedOptions).containsKey(OptionPriority.COMMAND_LINE); + List<ParsedOptionDescription> rcOptions = alpha.parsedOptions.get(OptionPriority.RC_FILE); + List<ParsedOptionDescription> cliOptions = alpha.parsedOptions.get(OptionPriority.COMMAND_LINE); + + assertThat(rcOptions).hasSize(2); + assertThat(rcOptions.get(0).getSource()).matches("rc file origin"); + assertThat(rcOptions.get(0).getUnconvertedValue()).matches("rc1"); + assertThat(rcOptions.get(1).getSource()).matches("rc file origin"); + assertThat(rcOptions.get(1).getUnconvertedValue()).matches("rc2"); + assertThat(cliOptions).hasSize(1); + assertThat(cliOptions.get(0).getSource()).matches("command line source"); + assertThat(cliOptions.get(0).getUnconvertedValue()).matches("cli"); } public static class CommaSeparatedOptionsExample extends OptionsBase { @@ -1646,10 +1715,42 @@ public class OptionsParserTest { @Test public void commaSeparatedOptionsWithAllowMultiple() throws Exception { OptionsParser parser = OptionsParser.newOptionsParser(CommaSeparatedOptionsExample.class); - parser.parse(OptionPriority.COMMAND_LINE, "a", Arrays.asList("--alpha=one", - "--alpha=two,three")); + parser.parse( + OptionPriority.COMMAND_LINE, + "command line source", + Arrays.asList("--alpha=one", "--alpha=two,three")); + parser.parse(OptionPriority.RC_FILE, "rc file origin", Arrays.asList("--alpha=rc1,rc2")); assertThat(parser.getOptions(CommaSeparatedOptionsExample.class).alpha) - .isEqualTo(Arrays.asList("one", "two", "three")); + .isEqualTo(Arrays.asList("rc1", "rc2", "one", "two", "three")); + } + + @Test + public void commaSeparatedListOptionsHaveCorrectPriorities() throws Exception { + OptionsParser parser = OptionsParser.newOptionsParser(CommaSeparatedOptionsExample.class); + parser.parse( + OptionPriority.COMMAND_LINE, + "command line source", + Arrays.asList("--alpha=one", "--alpha=two,three")); + parser.parse(OptionPriority.RC_FILE, "rc file origin", Arrays.asList("--alpha=rc1,rc2,rc3")); + + OptionValueDescription alphaValue = parser.getOptionValueDescription("alpha"); + assertThat(alphaValue).isInstanceOf(RepeatableOptionValueDescription.class); + + RepeatableOptionValueDescription alpha = (RepeatableOptionValueDescription) alphaValue; + assertThat(alpha.parsedOptions).containsKey(OptionPriority.RC_FILE); + assertThat(alpha.parsedOptions).containsKey(OptionPriority.COMMAND_LINE); + List<ParsedOptionDescription> rcOptions = alpha.parsedOptions.get(OptionPriority.RC_FILE); + List<ParsedOptionDescription> cliOptions = alpha.parsedOptions.get(OptionPriority.COMMAND_LINE); + + assertThat(rcOptions).hasSize(1); + assertThat(rcOptions.get(0).getSource()).matches("rc file origin"); + assertThat(rcOptions.get(0).getUnconvertedValue()).matches("rc1,rc2,rc3"); + assertThat(cliOptions).hasSize(2); + assertThat(cliOptions.get(0).getSource()).matches("command line source"); + assertThat(cliOptions.get(0).getUnconvertedValue()).matches("one"); + assertThat(cliOptions.get(1).getSource()).matches("command line source"); + assertThat(cliOptions.get(1).getUnconvertedValue()).matches("two,three"); + } public static class Yesterday extends OptionsBase { |