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

Generate zsh completions automatically #10403

Merged
merged 5 commits into from Jan 26, 2021

Conversation

Rylan12
Copy link
Member

@Rylan12 Rylan12 commented Jan 24, 2021

  • 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 typecheck with your changes locally?
  • Have you successfully run brew tests with your changes locally?
  • Have you successfully run brew man locally and committed any changes?

Continuation of the work for #10223.

This PR adds the ability for brew man to automatically generate zsh completions. Like with the bash completions, this isn't super intelligent at the moment. It doesn't know how to handle conflicting options or anything like that.

One thing that's different for zsh compared to bash completions is that zsh completions allow for descriptions to be used. Descriptions can be used for the brew commands themselves as well as for the options.

To automatically generate the descriptions for options, the description parameter defined for the option is used.

To generate descriptions for commands, I chose to use the first sentence of the command description. I skimmed through the generated descriptions to make sure they seemed reasonable but didn't look that closely. The way the first sentence is determined is by splitting the description by periods. There were a few cases where the first period was still within the first sentence of the description (when using i.e./e.g. or referring to a file with a dot in the path). I modified those command descriptions to not use periods within the first sentence.

@BrewTestBot
Copy link
Member

Review period will end on 2021-01-26 at 00:00:00 UTC.

@BrewTestBot BrewTestBot added the waiting for feedback Merging is blocked until sufficient time has passed for review label Jan 24, 2021
@Rylan12
Copy link
Member Author

Rylan12 commented Jan 24, 2021

One other thing I noticed is that the completions.rb file is starting to fill up. At the moment it handles linking/unlinking third-party completions as well as generating completions for two shells (soon to be a third). Each shell seems to need at least two helper methods plus a constant that maps named_arg types to functions.

It may be a good idea to split this up a little bit. One option is to have something like a CompletionsLinker and CompletionsGenerator module. Additionally or alternatively, the generation for the individual shells could be handled in separate files as well. I know we do something kind of similar with extend/os (the difference being that we only want to use one of the os extensions while we'd always want to use all of the shell completions extensions).

Thoughts on the best thing to do here?

@Rylan12
Copy link
Member Author

Rylan12 commented Jan 24, 2021

Okay, good news! I have fish completions working locally as well. Those require the changes in this PR but I'm going to hold off on pushing those, for now, to help reduce the size of this (already pretty big) PR.

@MikeMcQuaid
Copy link
Member

Additionally or alternatively, the generation for the individual shells could be handled in separate files as well.

This sounds good to me! Doesn't have to block this PR, though (in fact it may be easier to split up after this is merged). Good thinking about this 🎉

@Rylan12
Copy link
Member Author

Rylan12 commented Jan 25, 2021

This will be available to merge later today (once the conflicts are addressed). Should I go ahead and merge or would you rather I wait until after #10397 is merged per #10397 (review). Not sure what your estimated timing on that one is. Whichever is merged second will need to be rebased. It doesn't matter to me which one it is.

Rylan12 and others added 5 commits January 25, 2021 13:46
Don't have a period within first full sentence of the description
Co-Authored-By: Jonathan Chang <me@jonathanchang.org>
Co-Authored-By: Eric Knibbe <3324775+EricFromCanada@users.noreply.github.com>
@BrewTestBot BrewTestBot removed the waiting for feedback Merging is blocked until sufficient time has passed for review label Jan 26, 2021
@BrewTestBot
Copy link
Member

Review period ended.

@MikeMcQuaid
Copy link
Member

This will be available to merge later today (once the conflicts are addressed). Should I go ahead and merge or would you rather I wait until after #10397 is merged per #10397 (review). Not sure what your estimated timing on that one is. Whichever is merged second will need to be rebased. It doesn't matter to me which one it is.

Let's ship this first. Thanks again @Rylan12, great work!

@MikeMcQuaid MikeMcQuaid merged commit 00fab02 into Homebrew:master Jan 26, 2021
@Rylan12 Rylan12 deleted the zsh-completions branch January 26, 2021 15:05
@BrewTestBot BrewTestBot added the outdated PR was locked due to age label Feb 26, 2021
@Homebrew Homebrew locked as resolved and limited conversation to collaborators Feb 26, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
outdated PR was locked due to age
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants