[#964][#1080] ArgGroup synopsis should respect order (if specified)

Closes #964
remkop · Feb 14, 2022 · ee3f60b · ee3f60b
1 parent 20b3cda
commit ee3f60b
Show file tree

Hide file tree

Showing 3 changed files with 55 additions and 19 deletions.
diff --git a/RELEASE-NOTES.md b/RELEASE-NOTES.md
@@ -51,6 +51,7 @@ class MyIntConverter implements ITypeConverter<Integer> {
 * [#1380][#1505] API, bugfix: `requiredOptionMarker` should not be displayed on `ArgGroup` options. Thanks to [Ahmed El Khalifa](https://github.com/ahmede41) for the pull request.
 * [#1563] API: Add constructor to `PicocliSpringFactory` to allow custom fallback `IFactory`. Thanks to [Andrew Holland](https://github.com/a1dutch) for raising this.
 * [#1571] Enhancement: Variables in values from the default value provider should be interpolated. Thanks to [Bas Passon](https://github.com/bpasson) for raising this.
+* [#964][#1080] Enhancement: ArgGroup synopsis should respect order (if specified). Thanks to [Enderaoe](https://github.com/Lyther) for the pull request with unit tests.
 * [#1572] Enhancement: Remove redundant braces in ArgGroup synopsis.
 * [#1573] DEP: Bump JLine3 version to 3.21.0 from 3.19.0.
 

diff --git a/src/main/java/picocli/CommandLine.java b/src/main/java/picocli/CommandLine.java
@@ -10421,18 +10421,51 @@ private Text synopsisUnitText(Help.ColorScheme colorScheme, Text synopsis) {
             private Text rawSynopsisUnitText(Help.ColorScheme colorScheme, Set<ArgSpec> outparam_groupArgs) {
                 String infix = exclusive() ? " | " : " ";
                 Text synopsis = colorScheme.ansi().new Text(0);
-                for (ArgSpec arg : args()) {
-                    String prefix = synopsis.length > 0 ? infix : "";
-                    if (arg instanceof OptionSpec) {
-                        synopsis = concatOptionText(prefix, synopsis, colorScheme, (OptionSpec) arg);
-                    } else {
-                        synopsis = concatPositionalText(prefix, synopsis, colorScheme, (PositionalParamSpec) arg);
+
+                // if not sorted alphabetically then SortByOrder
+                boolean sortExplicitly = !args.isEmpty()
+                    && args.iterator().next().command() != null
+                    && !args.iterator().next().command().usageMessage().sortOptions();
+                if (sortExplicitly) {
+                    List<IOrdered> sortableComponents = new ArrayList<IOrdered>();
+                    List<PositionalParamSpec> remainder = new ArrayList<PositionalParamSpec>();
+                    for (ArgSpec arg : args()) {
+                        if (arg instanceof OptionSpec) {
+                            sortableComponents.add((IOrdered) arg);
+                        } else {
+                            remainder.add((PositionalParamSpec) arg);
+                        }
+                    }
+                    sortableComponents.addAll(subgroups());
+                    Collections.sort(sortableComponents, new Help.SortByOrder<IOrdered>());
+                    for (IOrdered ordered : sortableComponents) {
+                        String prefix = synopsis.length > 0 ? infix : "";
+                        if (ordered instanceof OptionSpec) {
+                            synopsis = concatOptionText(prefix, synopsis, colorScheme, (OptionSpec) ordered);
+                            outparam_groupArgs.add((OptionSpec) ordered);
+                        } else {
+                            synopsis = synopsis.concat(((ArgGroupSpec) ordered).synopsisText(colorScheme, outparam_groupArgs));
+                        }
+                    }
+                    for (PositionalParamSpec positional : remainder) {
+                        String prefix = synopsis.length > 0 ? infix : "";
+                        synopsis = concatPositionalText(prefix, synopsis, colorScheme, (PositionalParamSpec) positional);
+                        outparam_groupArgs.add(positional);
+                    }
+                } else {
+                    for (ArgSpec arg : args()) {
+                        String prefix = synopsis.length > 0 ? infix : "";
+                        if (arg instanceof OptionSpec) {
+                            synopsis = concatOptionText(prefix, synopsis, colorScheme, (OptionSpec) arg);
+                        } else {
+                            synopsis = concatPositionalText(prefix, synopsis, colorScheme, (PositionalParamSpec) arg);
+                        }
+                        outparam_groupArgs.add(arg);
+                    }
+                    for (ArgGroupSpec subgroup : subgroups()) {
+                        if (synopsis.length > 0) { synopsis = synopsis.concat(infix); }
+                        synopsis = synopsis.concat(subgroup.synopsisText(colorScheme, outparam_groupArgs));
                     }
-                    outparam_groupArgs.add(arg);
-                }
-                for (ArgGroupSpec subgroup : subgroups()) {
-                    if (synopsis.length > 0) { synopsis = synopsis.concat(infix); }
-                    synopsis = synopsis.concat(subgroup.synopsisText(colorScheme, outparam_groupArgs));
                 }
                 return synopsis;
             }

diff --git a/src/test/java/picocli/SynopsisOrderTest.java → .../picocli/OrderedArgGroupSynopsisTest.java b/src/test/java/picocli/SynopsisOrderTest.java → .../picocli/OrderedArgGroupSynopsisTest.java
@@ -8,39 +8,41 @@
 import picocli.CommandLine.ArgGroup;
 import picocli.CommandLine.Command;
 import picocli.CommandLine.Help;
+import picocli.CommandLine.Option;
 
 import static java.lang.String.format;
 import static org.junit.Assert.*;
 import static picocli.TestUtil.usageString;
 
-public class SynopsisOrderTest {
+// https://github.com/remkop/picocli/issues/964
+public class OrderedArgGroupSynopsisTest {
 	@Rule
 	public final ProvideSystemProperty ansiOFF = new ProvideSystemProperty("picocli.ansi", "false");
 	@Rule
 	public final SystemErrRule systemErrRule = new SystemErrRule().enableLog().muteForSuccessfulTests();
 
-	@Command()
+	@Command(sortOptions = false)
 	class SynopsisOrder {
-		class Group {
-			@CommandLine.Option(names = "--option1", required = true)
+		class GroupWithOneOption {
+			@Option(names = "--option1", required = true)
 			String value1;
 		}
 
 		class AllGroups {
-			@CommandLine.ArgGroup(exclusive = false, multiplicity = "1", order = 1) Group group;
-			@CommandLine.Option(names = "--option2", required = true, order = 2) String value2;
+			@ArgGroup(exclusive = false, multiplicity = "1", order = 1) GroupWithOneOption group;
+			@Option(names = "--option2", required = true, order = 2) String value2;
 		}
 
 		@ArgGroup(exclusive = true, multiplicity = "1")
 		AllGroups allGroups;
 	}
 
-    @Ignore("Requires #964")
+    //@Ignore("Requires #964")
 	@Test
 	public void testSynopsisOrderForArgGroup() {
 		String result = usageString(new SynopsisOrder(), Help.Ansi.OFF);
 		assertEquals(format("" +
-				"Usage: <main class> (--option1=<value1> | (--option2=<value2>))%n" +
+				"Usage: <main class> (--option1=<value1> | --option2=<value2>)%n" +
 				"      --option1=<value1>%n" +
 				"      --option2=<value2>%n"), result);
 	}