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

Colorful help (#340) and some help refactoring #3541

Closed
wants to merge 7 commits into from

Conversation

brodo
Copy link

@brodo brodo commented Dec 27, 2016

Hello,
this PR is related to issue #340 but also does some other things.

Subcommand map is now an array of CmdInfos

  • Having an array allows people to change the position of commands in the help text (maps are not ordered).
  • Commands now may have a command group. (needed for the main help message but also useful for other commands with many sub-commands)

HelpText now has an AdditionalHelp field

  • AdditionalHelp is displayed on the bottom of the help text. It is currently only used in the main help message, but may also be useful for other things.

There is now a --color command line flag

  • Colorful can be enabled with --color
  • There are no checks if stdout is a tty. If you use --color, you get color! (e.g. when someone wants to pipe it into less -R)

This PR touches a lot of files, but most changes are just simple gofmt transformations in order to switch from the Subcommand map to the Subcommand array.

Cheers from 33c3,
Julian

This allows custom command ordering and subcommand groups.

License: MIT
Signed-off-by: Julian Dax <julian.dax@me.com>
@brodo brodo force-pushed the develop branch 2 times, most recently from 374df2e to 4411faa Compare December 29, 2016 20:41
Julian Dax added 5 commits December 30, 2016 13:46
Commands can now have groups and are listed under these groups.
The output of $ipfs is also not longer using the `Subcommands` override.

License: MIT
Signed-off-by: Julian Dax <julian.dax@me.com>
License: MIT
Signed-off-by: Julian Dax <julian.dax@me.com>
License: MIT
Signed-off-by: Julian Dax <julian.dax@me.com>
License: MIT
Signed-off-by: Julian Dax <julian.dax@uni-siegen.de>
License: MIT
Signed-off-by: Julian Dax <julian.dax@uni-siegen.de>
@brodo brodo force-pushed the develop branch 3 times, most recently from 81dc4d4 to cfeacdd Compare December 30, 2016 13:24
License: MIT
Signed-off-by: Julian Dax <julian.dax@uni-siegen.de>
Copy link
Member

@whyrusleeping whyrusleeping left a comment

Choose a reason for hiding this comment

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

Hey @brodo, Thanks for the PR. I agree, the CLI is a bit... boring.

First off, we definitely want to separate features from refactoring. So the refactor from map to slice in subcommands should be its own PR, adding colors should be its own PR, and the additionalhelp field change needs to be its own PR.

Aside from that, we should find a way to test that the colors are rendering as expected in the sharness tests.

Copy link
Member

@Kubuxu Kubuxu left a comment

Choose a reason for hiding this comment

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

In my opinion the commands can be sorted by name for the display instead of making a map? Is there a case where it wouldn't be enough?

Would it be possible to instead make the Group string part of the Cmd struct itself instead of introducing new struct?

@@ -4,7 +4,9 @@ import (
"strings"
"testing"

"bytes"
Copy link
Member

Choose a reason for hiding this comment

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

this package should be joined with those above, this is stdlib package

@brodo
Copy link
Author

brodo commented Jan 6, 2017

@whyrusleeping Ok, I'll first divide this PR into three smaller ones. All the other things then can be discussed separately in the three PRs.

@brodo brodo closed this Jan 6, 2017
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.

3 participants