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

Add support for groups in the default HelpPrinter #135

Merged
merged 4 commits into from
Feb 8, 2021
Merged

Add support for groups in the default HelpPrinter #135

merged 4 commits into from
Feb 8, 2021

Conversation

mickael-menu
Copy link
Contributor

Fix #134

@alecthomas It is still lacking support for grouped flags, I wanted to check with you that this PR is welcome and on the right path first.

Here's what's implemented so far:

  • Ungrouped commands are displayed first, under a "Command:" group.
  • Each group is printed in order of appearance.
  • Tree mode ignores group for now.
    • This part is not so easy to implement, maybe that's a reasonable limitation.

I think it could be useful to add some group metadata as well. In this case, the group tag would be a key referencing a declared Group metadata. We could store:

  • The group title, in case it's long and to be able to modify it in only one spot.
    • Git headings are quite long, e.g. start a working area (see also: git help tutorial)
  • A short help message displayed under the group title.
    Filtering:
    Options used to filter the items in the list.
    
        --name   Filter by name
        --color   Filter by color
    

I'm not sure what would be the best place for such metadata. Maybe an Option?

model.go Outdated
@@ -203,6 +203,17 @@ func (n *Node) Path() (out string) {
return strings.TrimSpace(out)
}

// ClosestGroup finds the first non-empty group in this node and its ancestors.
func (n *Node) ClosestGroup() string {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I initially thought that subcommands would automatically inherit the group of their parents, from there:

kong/build.go

Lines 76 to 79 in 2479d83

// Assign parent if it's not already set.
if subf.tag.Group == "" {
subf.tag.Group = tag.Group
}

But subcommands are actually escaping the for loop from here:

kong/build.go

Line 62 in 2479d83

continue

I didn't want to mess too much with this part, so I added this ClosestGroup() function to do the job.

@alecthomas
Copy link
Owner

alecthomas commented Feb 6, 2021

Looks good!

That's a really great idea about the titles, the main reason I hadn't kept going with this is that putting long group titles in struct tags is not awesome.

I think, as you say, a function option such as kong.Groups({key: title}) would be a great solution.

@mickael-menu
Copy link
Contributor Author

Sounds good, I'll keep going then!

@mickael-menu mickael-menu marked this pull request as ready for review February 7, 2021 12:11
@mickael-menu
Copy link
Contributor Author

@alecthomas Ready for review, let me know if I missed something 👍

help.go Outdated
@@ -115,18 +116,18 @@ func printApp(w *helpWriter, app *Application) {
}
}

func printCommand(w *helpWriter, app *Application, cmd *Command) {
func printCommand(w *helpWriter, app *Application, cmd *Command, groups map[string]Group) {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Per other comment, the groups should be accessible through the model, in which case this should be unnecessary.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same thing for all the other locations.

options.go Outdated
@@ -208,6 +208,25 @@ func ConfigureHelp(options HelpOptions) Option {
})
}

// Group holds metadata about a node group used when printing help.
type Group struct {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should go into model.go, and be returned by the existing group related model methods.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just to clarify, you want something like this?

type Node struct {
    ...
-    Group string  
+    Group *Group
}

type Group struct {
+   // Key is the `group` field tag value used to identify this group.
+   Key string
    Title string
    Header string
}

And have the key -> Group resolved during parsing in build.go?

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Exactly.

})
expected := `Usage: test-app <command>

A test app.
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Formatting looks good!

@alecthomas
Copy link
Owner

Nice, thanks!

@alecthomas alecthomas merged commit b68e1ab into alecthomas:master Feb 8, 2021
@alecthomas
Copy link
Owner

This is awesome.

@mickael-menu mickael-menu deleted the grouping branch February 8, 2021 20:43
alecthomas added a commit that referenced this pull request Feb 9, 2021
@jotaen
Copy link
Contributor

jotaen commented Feb 13, 2021

🎉 This PR makes me really happy, it came exactly at the right time for me 😄 Thank you @mickael-menu!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Are logical groups still supported?
3 participants