Introduce subcommand grouping into the command configuration to improve help #644

DougGregor · 2024-06-02T23:44:53Z

Add optional support for grouping subcommands into named groups, to help bring order to commands with many subcommands without requiring additional structure. For example, here's the help for a "subgroupings" command that has an ungrouped subcommand (m), and two groups of subcommands ("broken" and "complicated").

USAGE: subgroupings <subcommand>

OPTIONS:
  -h, --help              Show help information.

SUBCOMMANDS:
  m

BROKEN SUBCOMMANDS:
  foo                     Perform some foo
  bar                     Perform bar operations

COMPLICATED SUBCOMMANDS:
  n

  See 'subgroupings help <subcommand>' for detailed help.

The help output above is created like this:

  struct WithSubgroups: ParsableCommand {
    static let configuration = CommandConfiguration(
      commandName: "subgroupings",
      subcommands: [ M.self ],
      groupedSubcommands: [
        CommandGroup(
          name: "Broken",
          subcommands: [ Foo.self, Bar.self ]
        ),
        CommandGroup(name: "Complicated", subcommands: [ N.self ]),
        CommandGroup(name: "Invisible", subcommands: [ ])
      ]
    )
  }

Each CommandGroup names a new group with its subcommands (there are no groups within groups).

This structure is only cosmetic, and only affects help generation by providing more structure for the reader. It doesn't impact existing clients, who can still reason about the flattened list of subcommands if they prefer.

…ve help Add optional support for grouping subcommands into named groups, to help bring order to commands with many subcommands without requiring additional structure. For example, here's the help for a "subgroupings" command that has an ungrouped subcommand (m), and two groups of subcommands ("broken" and "complicated"). USAGE: subgroupings <subcommand> OPTIONS: -h, --help Show help information. SUBCOMMANDS: m BROKEN SUBCOMMANDS: foo Perform some foo bar Perform bar operations COMPLICATED SUBCOMMANDS: n See 'subgroupings help <subcommand>' for detailed help. To be able freely mix subcommands and subcommand groups, CommandConfiguration has a new initializer that takes a result builder. The help output above is created like this: struct WithSubgroups: ParsableCommand { static let configuration = CommandConfiguration( commandName: "subgroupings" ) { CommandGroup(name: "Broken") { Foo.self Bar.self } M.self CommandGroup(name: "Complicated") { N.self } } } Each `CommandGroup` names a new group and is given commands (there are no groups within groups). The other entries are arbitrary ParsableCommands. This structure is only cosmetic, and only affects help generation by providing more structure for the reader. It doesn't impact existing clients, who can still reason about the flattened list of subcommands if they prefer.

DougGregor · 2024-06-02T23:44:59Z

@swift-ci please test

dickoff · 2024-06-03T03:20:52Z

I was just looking at doing something very similar! (WIP here: https://github.com/apple/swift-argument-parser/tree/subcommand-groups).

The CommandsBuilder idea is an interesting one. However is it limiting in some of the manipulation that you can do with the existing simple array subcommands parameter? Currently it's quite easy to do things like dynamically alphabetically sort the subcommands array or only include some commands based on #if checks. I checked out this branch and played around with a it a bit and it wasn't obvious to me how you could achieve those kinds of things with the builder (but I'm very inexperienced with resultBuilders!)

dickoff · 2024-06-03T03:25:49Z

Do we think there's value in allowing for a longer description string for the group? For example how subcommand groups are described in git -h. E.g. accomplishing something like this in the test, adding a description to the header title.

BROKEN SUBCOMMANDS: These commands are currently non-functional due to XYZ
  foo                     Perform some foo
  bar                     Perform bar operations

DougGregor · 2024-06-03T04:15:47Z

The CommandsBuilder idea is an interesting one. However is it limiting in some of the manipulation that you can do with the existing simple array subcommands parameter?

It shouldn't be too limiting. Result builders can support more language syntax if we'd like (e.g., the ability to do if #available and such), although I didn't add them.

I checked out this branch and played around with a it a bit and it wasn't obvious to me how you could achieve those kinds of things with the builder (but I'm very inexperienced with resultBuilders!)

I'd expect #if to work inside the builder already. If we want if, if-else, switch, and if #available to work, we can add that support to the builder by introducing the appropriate buildXYZ methods.

Do we think there's value in allowing for a longer description string for the group?

Yes, I think that's a good idea! It's easy enough to add an "abstract" that we put here.

…syntax Adds support for if, if-else, if #available, and for..in loops.

DougGregor · 2024-06-03T05:02:48Z

@swift-ci please test

DougGregor · 2024-06-03T05:04:40Z

@dickoff See the latest two commits, which add an optional abstract and then extend out the result builders to handle all of the declarative syntax.

DougGregor · 2024-06-03T05:15:32Z

I've opened up a discussion thread on the forums for this change: https://forums.swift.org/t/grouping-subcommands/72219

dickoff · 2024-06-03T17:16:26Z

@dickoff See the latest two commits, which add an optional abstract and then extend out the result builders to handle all of the declarative syntax.

New changes look great! They resolve all the questions I brought up.

rauhul · 2024-06-03T17:56:47Z

I would recommend splitting out the abstract into a different PR. If we add this functionality to CommandGroups then we should probably add them OptionGroups and that feels like scope creep for this change.

natecook1000

Thanks so much for this, @DougGregor! In addition to the notes below, could you add initializers that just accept arrays that are parallel to the builder-based initializers for CommandConfiguration and CommandGroup? That will be more flexible for projects that set up the command hierarchy out of place with the command declarations themselves.

natecook1000 · 2024-06-03T17:14:51Z

Sources/ArgumentParser/Usage/HelpGenerator.swift

+      var renderedHeader = String(describing: header).uppercased() + ":"
+      if let abstract = header.abstract {
+        renderedHeader += " "
+        renderedHeader += abstract.wrapped(to: screenWidth)


Do we need to add the abstract and then wrap?

Ahhh, probably. Thank you

natecook1000 · 2024-06-03T17:16:46Z

Sources/ArgumentParser/Usage/HelpGenerator.swift

+    // - grouped subcommands
+    // - ungrouped subcommands


Looks like this order is flipped from the actual behavior.

natecook1000 · 2024-06-03T17:25:21Z

Sources/ArgumentParser/Parsable Types/SubcommandsBuilder.swift

+}
+
+/// Describes a single subcommand or a group thereof.
+public enum Subcommand: Sendable {


This name doesn't quite describe what the type represents, since a group of subcommands is not itself a subcommand. One option is CommandGroup – I'm more comfortable with a CommandGroup that just stores a single command than a Subcommand that stores a bunch of commands. But then we couldn't use that name for the other type, which it's perfect for.

Maybe just making it Subcommands is an improvement? I don't think this is a dealbreaker – do you have any other ideas for this name?

Tests/ArgumentParserUnitTests/HelpGenerationTests.swift

natecook1000 · 2024-06-03T17:54:39Z

Sources/ArgumentParser/Parsable Types/CommandConfiguration.swift

+  ///     are `-h` and `--help`.
+  ///   - aliases: An array of aliases for the command's name. All of the aliases
+  ///     MUST not match the actual command name, whether that be the derived name
+  ///     if `commandName` is not provided, or `commandName` itself if provided.


Something like this:

Suggested change

/// if `commandName` is not provided, or `commandName` itself if provided.

/// if `commandName` is not provided, or `commandName` itself if provided.

/// - groupedSubcommands: A builder closure that defines the subcommands

/// and their groups for this command.

DougGregor · 2024-06-03T18:50:48Z

could you add initializers that just accept arrays that are parallel to the builder-based initializers for CommandConfiguration and CommandGroup?

Hmm. If we're going to do this, I think I'd rather drop the builder-based initializers entirely from this pull request than introduce two new interfaces for the same thing. It's completely reasonable to say that the only way to get this functionality is with syntax like this:

subcommands: [
  // all of the ungrouped subcommands
],
groupedSubcommands: [
  CommandGroup(
    name: "group name",
    subcommands: [
      // subcommands in this group
    ]
  ),
  // ...
].

That avoids having to create the Subcommand type at all, and is arguably more in keeping with the current API design. It would leave the door open to a builder-based design in the future should we want it.

natecook1000 · 2024-06-03T19:31:24Z

That makes sense! I could see that fitting into a whole builder-based configuration approach (which I think we've looked at before). If we make this change, let's also make flattenedSubcommands (or allSubcommands) public API as well, since the subcommands will live in two different places.

dickoff · 2024-06-03T20:07:46Z

That makes sense! I could see that fitting into a whole builder-based configuration approach (which I think we've looked at before). If we make this change, let's also make flattenedSubcommands (or allSubcommands) public API as well, since the subcommands will live in two different places.

Could we not just have the existing subcommands still return all ungrouped and grouped subcommands together? Having subcommands start to only return ungrouped commands would be odd for clients I would think.

DougGregor · 2024-06-03T21:25:41Z

Could we not just have the existing subcommands still return all ungrouped and grouped subcommands together?

I think we need subcommands to still return all ungrouped and grouped subcommands. I can add ungroupedSubcommands and groupedSubcommands for clients that want them separate.

This reverts commit ab563a2.

…rray Introduce subcommand groups with a more modest extension to the API that adds another array of subcommand groups alongside the (ungrouped) subcommands array. We can consider introducing result builders as a separate step later, if there's more to be gained from it.

DougGregor · 2024-06-04T05:02:52Z

@swift-ci please test

DougGregor · 2024-06-04T05:04:13Z

@natecook1000 I believe I addressed all of your comments. The updated PR description contains the non-result-builder interface, which is a more straightforward extension of the existing syntax:

  struct WithSubgroups: ParsableCommand {
    static let configuration = CommandConfiguration(
      commandName: "subgroupings",
      subcommands: [ M.self ],
      groupedSubcommands: [
        CommandGroup(
          name: "Broken",
          subcommands: [ Foo.self, Bar.self ]
        ),
        CommandGroup(name: "Complicated", subcommands: [ N.self ]),
        CommandGroup(name: "Invisible", subcommands: [ ])
      ]
    )
  }

rauhul

Looks like a great addition and UX improvement.

rauhul · 2024-06-04T15:49:29Z

Sources/ArgumentParser/Parsable Types/CommandConfiguration.swift

@@ -47,8 +47,27 @@ public struct CommandConfiguration: Sendable {
  public var shouldDisplay: Bool

  /// An array of the types that define subcommands for this command.
-  public var subcommands: [ParsableCommand.Type]
-
+  ///


Can we drop this stray doc comment line?

I was using the extra line to separate the "abstract" line from the "detail" line.

Wow I can't this morning, I made like 2 other mistakes similar, sorry about the noise

It's a weird diff!

natecook1000

Looks great, thanks again!

natecook1000 · 2024-06-04T21:18:13Z

Sources/ArgumentParser/Parsable Types/CommandConfiguration.swift

+  /// An array of types that define subcommands for this command and are
+  /// not part of any command group.
+  public var ungroupedSubcommands: [ParsableCommand.Type]
+
+  /// The list of subcommands and subcommand groups.
+  public var groupedSubcommands: [CommandGroup]


These look great 👍🏻

rauhul linked an issue Jun 3, 2024 that may be closed by this pull request

Add ability to group commands #643

Closed

DougGregor added 2 commits June 2, 2024 21:45

Add an optional abstract to command groups

ab563a2

Expand subcommand group result builders to handle all result-builder …

03cd075

…syntax Adds support for if, if-else, if #available, and for..in loops.

natecook1000 reviewed Jun 3, 2024

View reviewed changes

DougGregor added 3 commits June 3, 2024 21:29

Revert "Add an optional abstract to command groups"

e51daba

This reverts commit ab563a2.

Drop the (ungrouped) "subcommands" heading when there are none.

6c2ab1a

rauhul approved these changes Jun 4, 2024

View reviewed changes

natecook1000 approved these changes Jun 4, 2024

View reviewed changes

DougGregor merged commit 516c2f8 into main Jun 4, 2024
2 checks passed

DougGregor deleted the command-groups-help branch June 4, 2024 21:33

Introduce subcommand grouping into the command configuration to improve help #644

Introduce subcommand grouping into the command configuration to improve help #644

Uh oh!

Conversation

DougGregor commented Jun 2, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DougGregor commented Jun 2, 2024

Uh oh!

dickoff commented Jun 3, 2024

Uh oh!

dickoff commented Jun 3, 2024

Uh oh!

DougGregor commented Jun 3, 2024

Uh oh!

DougGregor commented Jun 3, 2024

Uh oh!

DougGregor commented Jun 3, 2024

Uh oh!

DougGregor commented Jun 3, 2024

Uh oh!

dickoff commented Jun 3, 2024

Uh oh!

rauhul commented Jun 3, 2024

Uh oh!

natecook1000 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DougGregor commented Jun 3, 2024

Uh oh!

natecook1000 commented Jun 3, 2024

Uh oh!

dickoff commented Jun 3, 2024

Uh oh!

DougGregor commented Jun 3, 2024

Uh oh!

DougGregor commented Jun 4, 2024

Uh oh!

DougGregor commented Jun 4, 2024

Uh oh!

rauhul left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

natecook1000 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

DougGregor commented Jun 2, 2024 •

edited

Loading