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

completions: make opt-in only #10268

Merged
merged 8 commits into from Jan 13, 2021
Merged

Conversation

Rylan12
Copy link
Member

@Rylan12 Rylan12 commented Jan 8, 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?

This PR adds a brew completions command and makes linking completions opt-in only per discussion in #10229.

Once this PR is merged, users will see a message in brew update informing them that completions are no longer linked. It will direct them to run brew completions link to link completions. The user can choose whether completions are linked by running brew completions link or brew completions unlink. They can check whether completions are linked by running brew completions or brew completions status.

CC @MikeMcQuaid

@BrewTestBot
Copy link
Member

Review period will end on 2021-01-11 at 16:14:49 UTC.

@BrewTestBot BrewTestBot added the waiting for feedback Merging is blocked until sufficient time has passed for review label Jan 8, 2021
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.

Thanks @Rylan12! Sorry if my communication around this has been sub-optimal.

Library/Homebrew/cmd/update-report.rb Outdated Show resolved Hide resolved
Library/Homebrew/test/cmd/completions_spec.rb Outdated Show resolved Hide resolved
docs/Shell-Completion.md Outdated Show resolved Hide resolved
@Rylan12
Copy link
Member Author

Rylan12 commented Jan 11, 2021

Ah, looks like I did misunderstand a little. You're saying we should automatically link completions for built-in commands just not for third-party taps, right?

If so, this PR isn't quite ready yet because it stops all linking.

@MikeMcQuaid
Copy link
Member

Ah, looks like I did misunderstand a little. You're saying we should automatically link completions for built-in commands just not for third-party taps, right?

Yes, sorry!

@MikeMcQuaid
Copy link
Member

Ah, looks like I did misunderstand a little. You're saying we should automatically link completions for built-in commands just not for third-party taps, right?

Essentially: if we (Homebrew) control the code: it's enabled by default. If we don't: it requires some user action.

@BrewTestBot
Copy link
Member

Review period ended.

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

Rylan12 commented Jan 11, 2021

Okay, I've updated to still link completions from official taps automatically.

Additionally, the message only shows if there are completions in non-official taps available.

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.

Looking good! Some wording changes.

Library/Homebrew/completions.rb Outdated Show resolved Hide resolved
Library/Homebrew/completions.rb Outdated Show resolved Hide resolved
Library/Homebrew/cmd/completions.rb Outdated Show resolved Hide resolved
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.

Thanks @Rylan12, great work! Some comments but all optional/non-blocking.

Library/Homebrew/completions.rb Outdated Show resolved Hide resolved
Library/Homebrew/settings.rb Outdated Show resolved Hide resolved
@@ -22,7 +22,7 @@ RSpec/InstanceVariable:
- 'utils/git_spec.rb'
- 'version_spec.rb'

# Offense count: 76
# Offense count: 81
Copy link
Member

Choose a reason for hiding this comment

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

At this point: let's either not use multiple describes in the test or disable this cop.

Copy link
Member Author

@Rylan12 Rylan12 Jan 13, 2021

Choose a reason for hiding this comment

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

Which is preferable? It would be relatively easy to just add a single describe that encompasses the entire file for each of these. I don't think it would cause much additional clutter at least to the cmd/*, dev-cmd/*, rubocops/* files. I haven't looked at the others.

Alternatively, we can cut down the number of exclusions by using:

Exclude:
  - 'ENV_spec.rb'
  - 'cleanup_spec.rb'
  - 'cmd/*.rb'
  - 'dependency_spec.rb'
  - 'dev-cmd/*.rb'
  - 'download_strategies_spec.rb'
  - 'exceptions_spec.rb'
  - 'formula_support_spec.rb'
  - 'inreplace_spec.rb'
  - 'language/python_spec.rb'
  - 'options_spec.rb'
  - 'os/mac/mach_spec.rb'
  - 'patch_spec.rb'
  - 'rubocops/*.rb'
  - 'software_spec_spec.rb'
  - 'tap_spec.rb'
  - 'utils/git_spec.rb'
  - 'version_spec.rb'

That doesn't really reduce the number of offenses it just cleans up the exclusion list.

Can this go in a separate PR (which I'm happy to do)?

For now, I'll add new files with only a single describe so this isn't increased (although the reason the number jumped so much here is not that I added 5 new offenses, it was just out of date. I only added one)

Copy link
Member

Choose a reason for hiding this comment

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

Which is preferable? It would be relatively easy to just add a single describe that encompasses the entire file for each of these. I don't think it would cause much additional clutter at least to the cmd/*, dev-cmd/*, rubocops/* files. I haven't looked at the others.

Either is fine with me. Slight preference for "doing what the cop wants".

Can this go in a separate PR (which I'm happy to do)?

Definitely (and thanks for offering)!

For now, I'll add new files with only a single describe so this isn't increased (although the reason the number jumped so much here is not that I added 5 new offenses, it was just out of date. I only added one)

Perfect 👍🏻

@Rylan12 Rylan12 merged commit 7019899 into Homebrew:master Jan 13, 2021
@Rylan12 Rylan12 deleted the completions-opt-out branch January 13, 2021 21:08
@Rylan12 Rylan12 mentioned this pull request Jan 31, 2021
8 tasks
@BrewTestBot BrewTestBot added the outdated PR was locked due to age label Feb 13, 2021
@Homebrew Homebrew locked as resolved and limited conversation to collaborators Feb 13, 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

5 participants