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

cli_parser: man-pages and help text generation using Homebrew::CLI::Parser #4383

Merged
merged 18 commits into from Oct 3, 2018

Conversation

GauthamGoli
Copy link
Contributor

@GauthamGoli GauthamGoli commented Jun 28, 2018

  • Have you followed the guidelines in our Contributing document?
  • Have you checked to ensure there aren't other open Pull Requests for the same change?
  • Have you added an explanation of what your changes do and why you'd like us to include them?
  • Have you written new tests for your changes? Here's an example.
  • Have you successfully run brew style with your changes locally?
  • Have you successfully run brew tests with your changes locally?

#4271 (comment)
This PR is about generating --help text using OptionParser and to get feedback on the interface, and the way help text is being shown as its different from what we currently show.

brew audit --help would return something like this.

Usage: brew audit [options] [<formulae>]

Check <formulae> for Homebrew coding style violations. This should be
run before submitting a new formula.

If no <formulae> are provided, all of them are checked.
        --strict                     Run additional style checks, including Rubocop style checks.
        --online                     Run additional slower style checks that require a
                                     network connection.
        --new-formula                Run various additional style checks to determine if a new formula
                                     is eligible for Homebrew. This should be used when creating
                                     new formula and implies --strict and --online.
        --fix                        Fix style violations automatically using
                                     RuboCop's auto-correct feature.
        --display-cop-names          Include the RuboCop cop name for each violation
                                     in the output.
        --display-filename           Prefix everyline of output with name of the file or
                                     formula being audited, to make output easy to grep.
    -D, --audit-debug                Activates debugging and profiling
        --only                       Passing --only=method will run only the methods named audit_method,
                                     `method` should be a comma-separated list.
        --except                     Passing --except=method will run only the methods named audit_method,
                                     `method` should be a comma-separated list.
        --only-cops                  Passing --only-cops=cops will check for violations of only the listed
                                     RuboCop cops. `cops` should be a comma-separated list of cop names.
        --except-cops                Passing --except-cops=cops will skip checking the listed
                                     RuboCop cops violations. `cops` should be a comma-separated list of cop names.
    -v, --verbose                    Verbose mode.
    -d, --debug                      Display debug info.
    -h, --help                       Show this message

@ghost ghost assigned GauthamGoli Jun 28, 2018
@ghost ghost added the in progress Maintainers are working on this label Jun 28, 2018
@MikeMcQuaid
Copy link
Member

Can you run brew man so we can see the different help text output? Thanks!

@@ -82,7 +82,7 @@
# - a help flag is passed AND there is no command specified
# - no arguments are passed
# - if cmd is Cask, let Cask handle the help command instead
if (empty_argv || help_flag) && cmd != "cask"
if (empty_argv || help_flag) && cmd != "cask" && !internal_dev_cmd
Copy link
Member

Choose a reason for hiding this comment

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

Can you explain this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Developer Command level OptionParser would handle respective --help text generation. Until now Homebrew::Help.help cmd would be generating the help text.

And we have OptionParser enabled only for developer commands. Hence the override.

Copy link
Member

Choose a reason for hiding this comment

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

I see. I think it would be good to make this part of the Homebrew::Help.help class instead of here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah. I'll try doing that.

Copy link
Member

Choose a reason for hiding this comment

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

Also CC @reitermarkus as a note that it'd be good to get Cask's help behaving similar to the rest of Homebrew's.


If no <formulae> are provided, all of them are checked.
EOS
switch "--strict", description: "Run additional style checks, including Rubocop style checks."
Copy link
Member

Choose a reason for hiding this comment

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

What do you think about making the second argument default to a description here? The description: feel maybe a bit redundant.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

switch's first two arguments are already being specified for short and long options . eg:
switch "-s", "--strict"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

May be we can separate them out by checking if they start with - and doesn't contain any

Copy link
Member

Choose a reason for hiding this comment

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

Maybe short options could be short: "-s" or something instead? They'll be used less than descriptions, right?

switch "--strict", description: "Run additional style checks, including Rubocop style checks."
switch "--online", description: "Run additional slower style checks that require a\nnetwork connection."
switch "--new-formula", description: "Run various additional style checks to determine if a new formula \nis eligible for Homebrew. This should be used when creating \nnew formula and implies --strict and --online."
switch "--fix", description: "Fix style violations automatically using\nRuboCop's auto-correct feature."
Copy link
Member

Choose a reason for hiding this comment

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

<<~EOS is preferable to \n here.

@GauthamGoli
Copy link
Contributor Author

I've been caught up at work.. will get back working on this in 3 days.

@GauthamGoli
Copy link
Contributor Author

@MikeMcQuaid From what I see, OptionParser is very well suited for generating brew <command> --help

Man pages are formatted differently, we cannot directly use the help text as-is, but it CAN be used by writing some extra code. I looked at the man pages of git which also has sub-commands like brew

Before proceeding, I have a question to clarify on help text rephrasing.

  • brew audit --help generated without OptionParser
brew audit [--strict] [--fix] [--online] [--new-formula] [--display-cop-names] [--display-filename] [--only=method|--except=method] [--only-cops=cops|--except-cops=cops] [formulae]:
    Check formulae for Homebrew coding style violations. This should be
    run before submitting a new formula.

    If no formulae are provided, all of them are checked.

    If --strict is passed, additional checks are run, including RuboCop
    style checks.

(snipped)

  • brew audit --help generated with OptionParser
brew audit [options] [<formulae>]

        Check <formulae> for Homebrew coding style violations. This should be
        run before submitting a new formula.

        If no <formulae> are provided, all of them are checked.

        --strict                     Run additional style checks, including Rubocop style checks.

Since OptionParser displays the description adjacent to the option, I rephrased it for brevity from

If --strict is passed, additional checks are run, including RuboCop style checks.

to

Run additional style checks, including Rubocop style checks.

Do we want to keep the current help text as-is , or rephrase it to latter ?

@MikeMcQuaid
Copy link
Member

Cool, looks good thanks @GauthamGoli.

rephrase it to latter ?

This would be great 👍

@MikeMcQuaid
Copy link
Member

This doesn't look bad but would be nice to have some of the nicer formatting tweaks:

screenshot 2018-08-27 at 10 42 59

On a narrower terminal the existing version wraps a bit nicer:

screenshot 2018-08-27 at 10 43 52

@GauthamGoli
Copy link
Contributor Author

I was facing formatting issues and had given up temporarily. I will give it a try again now to get the formatting right.

@MikeMcQuaid
Copy link
Member

@GauthamGoli Thanks! We could always write our own generator if that'd be better.

@GauthamGoli
Copy link
Contributor Author

GauthamGoli commented Sep 8, 2018

@MikeMcQuaid Can you please generate manpage and let me know what you think? (Wrote a simple generator, I think its better now)

@MikeMcQuaid
Copy link
Member

Can you please generate manpage and let me know what you think? (Wrote a simple generator, I think its better now)

@GauthamGoli Yeh, looks nicer. Could still be tweaked further I think but that's a big step up on what there was before 👍

@GauthamGoli
Copy link
Contributor Author

GauthamGoli commented Sep 9, 2018

Could still be tweaked further

@MikeMcQuaid Let me know at what places? I'll do it and lets ship this finally, its been too long this PR has been open 😅 😅

@@ -31,6 +31,10 @@ def error(string, label: nil)
label(label, string, :red)
end

def wrap(s, width = 172, indentation = 0)
" "*indentation + s.gsub(/(.{1,#{width}})(\s+|\Z)/, "\\1\n").gsub("\n", "\n"+" "*indentation) + "\n"
Copy link
Member

Choose a reason for hiding this comment

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

This is hard to follow. Could you use intermediate variables and/or string interpolation and put each .gsub on a new line?

end

def wrap_option_desc(desc)
Formatter.wrap(desc, @desc_line_length).split("\n")
Copy link
Member

Choose a reason for hiding this comment

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

What happens without these being wrapped? Is it the manpage/markdown/something else that looks weird?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If not wrapped this way, In the help text , option description would be in a single line and wrapped based on terminal's width and new line starts without any indentation which would look weird like below

        --online                     Run additional slower style checks that require a
network connection. 

When lines are wrapped and passed as an array, OptionParser's help text generator would show it with proper indentation.

        --online                     Run additional slower style checks that require a
                                     network connection. 

Manpage formatting is handled separately in man.rb itself.

Copy link
Member

Choose a reason for hiding this comment

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

@GauthamGoli I see, thanks 👍

@@ -34,9 +48,18 @@ def switch(*names, description: nil, env: nil, required_for: nil, depends_on: ni
enable_switch(*names) if !env.nil? && !ENV["HOMEBREW_#{env.to_s.upcase}"].nil?
end

def banner(text)
Copy link
Member

Choose a reason for hiding this comment

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

Is the banner the help text? If so, a name relating to that might be more obvious.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Banner is a brief description of the command shown above the options.
Ex:

      banner <<~EOS
         Usage: brew audit [options] [<formulae>]
         Check <formulae> for Homebrew coding style violations. This should be
         run before submitting a new formula.
         If no <formulae> are provided, all of them are checked.
      EOS

Copy link
Member

Choose a reason for hiding this comment

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

@GauthamGoli Ok, I see. Maybe usage_banner or help_banner or similar?

when :verbose then [["-v", "--verbose"], :verbose]
when :debug then [["-d", "--debug"], :debug]
when :force then [["-f", "--force"], :force]
when :quiet then [["-q", "--quiet"], :quiet, "Suppress warnings."]
Copy link
Member

Choose a reason for hiding this comment

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

Suppress any warnings

when :force then [["-f", "--force"], :force]
when :quiet then [["-q", "--quiet"], :quiet, "Suppress warnings."]
when :verbose then [["-v", "--verbose"], :verbose, "Verbose mode."]
when :debug then [["-d", "--debug"], :debug, "Display debug info."]
Copy link
Member

Choose a reason for hiding this comment

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

Display any debugging information

when :debug then [["-d", "--debug"], :debug]
when :force then [["-f", "--force"], :force]
when :quiet then [["-q", "--quiet"], :quiet, "Suppress warnings."]
when :verbose then [["-v", "--verbose"], :verbose, "Verbose mode."]
Copy link
Member

Choose a reason for hiding this comment

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

Make some output more verbose

when :quiet then [["-q", "--quiet"], :quiet, "Suppress warnings."]
when :verbose then [["-v", "--verbose"], :verbose, "Verbose mode."]
when :debug then [["-d", "--debug"], :debug, "Display debug info."]
when :force then [["-f", "--force"], :force, "Override any warnings/validations."]
Copy link
Member

Choose a reason for hiding this comment

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

Override warnings and enable potentially unsafe operations

EOS
switch "--strict", description: "Run additional style checks, including Rubocop style checks."
switch "--online", description: "Run additional slower style checks that require a network connection."
switch "--new-formula", description: "Run various additional style checks to determine if a new formula is eligible for Homebrew. This should be used when creating new formula and implies --strict and --online."
Copy link
Member

Choose a reason for hiding this comment

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

This line (and some others) is a bit too long. Maybe stick each description: on a new line?

@MikeMcQuaid
Copy link
Member

@GauthamGoli Good work on this! I think ideally if we're writing our own output I'd just aim for the output to be as close as possible to the existing rendered manpage output i.e. do stuff like If X is passed, and indent/wrap similarly to the existing. Anywhere where that seems to be fair too much work: shout here and we can talk about whether it's worth it.

@GauthamGoli
Copy link
Contributor Author

GauthamGoli commented Sep 12, 2018

@MikeMcQuaid The current manpage output for brew audit command is similar to the manpage of git and nmap (I like this format). I've noticed few other command line tools use this similar formatting in manpages instead of If X is passed, format.

Do you think its worth transitioning to this new format ? If not, I'll make it as close to existing format.

@MikeMcQuaid
Copy link
Member

Do you think its worth transitioning to this new format ?

Yes, let's just do it 👍. Looking at the git format:

  • Usage: brew audit [options] [formulae] could be instead audit [options] [formulae] and bolded/coloured the same in the manpage as they are currently (formulae is coloured but options is not for some reason).

@GauthamGoli
Copy link
Contributor Author

GauthamGoli commented Sep 15, 2018

@MikeMcQuaid addressed the comments. Please take a look when you get time! If this code style is OK, I'll do the same for other developer commands.

Do we plan to remove the comments in the beginning of every command.rb that we previously used to generate help and manpages with?


EOS
switch "--strict", description: "Run additional style checks, "\
"including Rubocop style checks."
Copy link
Member

Choose a reason for hiding this comment

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

What about:

switch      "--strict",
  description: "Run additional style checks, including Rubocop style checks."

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In the current style, one could see all the declared options in a single column (I think its more readable this way but with trade off of having description spanning multiple lines), but with above style the description would come in between. I would defer the final call on this to you @MikeMcQuaid

Copy link
Member

Choose a reason for hiding this comment

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

I think it'd be preferable to be able to have the description on a single line, thanks 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@MikeMcQuaid I've updated, can you see if this is okay?

@MikeMcQuaid
Copy link
Member

Do we plan to remove the comments in the beginning of every command.rb that we previously used to generate help and manpages with?

Provided e.g. brew audit --help works without it: yes.

If this code style is OK, I'll do the same for other developer commands.

The manpage style is perfect. I agree it's actually better than the current style now 😍. There will be some slight inconsistency when the previous command style is used just for non-developer commands but I think it's worth it to get this merged.

Copy link
Member

@MikeMcQuaid MikeMcQuaid left a comment

Choose a reason for hiding this comment

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

brew audit --help seems to be still running the audit command. Additionally, it'd be nice if the options were bolded there. Other than that 👍 👏 🎉

run before submitting a new formula.

If no <formulae> are provided, all of them are checked.

Copy link
Member

Choose a reason for hiding this comment

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

Might be nice to make it so you don't need this trailing empty line?

If no <formulae> are provided, all of them are checked.

EOS
switch "--strict",
Copy link
Member

Choose a reason for hiding this comment

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

Sorry, terrible pedantry now but given the way description is I'd be tempted to say that switch "--strict", would look better.

@GauthamGoli
Copy link
Contributor Author

GauthamGoli commented Sep 28, 2018

@MikeMcQuaid Now manpages for all dev commands are being generated through this. Can you once take a look, and let me know your thoughts? Thanks.

There are minor hiccups like --verbose and --debug appearing for every sub-command.

@MikeMcQuaid
Copy link
Member

screenshot 2018-09-30 at 10 56 11

@GauthamGoli I think it would be good if there was a bit more visual separation between commands either through indentation, keeping options on the same line as their first line of description or something else.

There are minor hiccups like --verbose and --debug appearing for every sub-command.

Agreed, would be good to resolve this before merge. There could be a "global options" or similar section that apply to all commands.

@GauthamGoli
Copy link
Contributor Author

@MikeMcQuaid Is the indentation fine now? I made the command synopsis to be H3 ( ###)
(but I'm not able to get the usual underline formatting to work in the sub heading. )

@MikeMcQuaid
Copy link
Member

Is the indentation fine now?

Looks great 😍.

More pedantry:
screenshot 2018-10-01 at 14 26 16

  • Ideally [options] would have only options in red (or maybe green? see install-options and not the [ ]
  • Ideally formulae would be in green (and have the same [] fixes)
  • As mentioned above: ideally --verbose, --debug wouldn't be repeated

After that: I think we're good to go. So close now! Great work again @GauthamGoli!

@GauthamGoli
Copy link
Contributor Author

GauthamGoli commented Oct 2, 2018

@MikeMcQuaid Now --verbose and other global options have their own section in manpages and won't be shown for every command.
For audit [options] [formulae] , I am not able to get the usual formatting to work may be because its H3, ronn is creating some tags. While generating man pages, I'm getting rid of markdown formatting here so that audit [options] [formulae] appears in the manpages as-is.

@GauthamGoli
Copy link
Contributor Author

@MikeMcQuaid any idea why the build is failing?

@MikeMcQuaid
Copy link
Member

@GauthamGoli Fixing in Homebrew/homebrew-test-bot#201 then will rerun.

@GauthamGoli GauthamGoli changed the title audit: Use OptionParser to generate help text cli_parser: man-pages and help text generation using Homebrew::CLI::Parser Oct 3, 2018
Homebrew TODO automation moved this from Work In Progress to Review Approved Oct 3, 2018
@MikeMcQuaid MikeMcQuaid merged commit c97d3b0 into Homebrew:master Oct 3, 2018
Homebrew TODO automation moved this from Review Approved to Done Oct 3, 2018
@MikeMcQuaid
Copy link
Member

Great work again @GauthamGoli 🎉

@lock lock bot added the outdated PR was locked due to age label Nov 2, 2018
@lock lock bot locked as resolved and limited conversation to collaborators Nov 2, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
in progress Maintainers are working on this outdated PR was locked due to age
Projects
Homebrew TODO
  
Done
Development

Successfully merging this pull request may close these issues.

None yet

3 participants