-
-
Notifications
You must be signed in to change notification settings - Fork 10.7k
completions: make opt-in only #10268
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
Conversation
|
Review period will end on 2021-01-11 at 16:14:49 UTC. |
MikeMcQuaid
left a comment
There was a problem hiding this 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.
|
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. |
Yes, sorry! |
Essentially: if we (Homebrew) control the code: it's enabled by default. If we don't: it requires some user action. |
|
Review period ended. |
|
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. |
MikeMcQuaid
left a comment
There was a problem hiding this 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.
MikeMcQuaid
left a comment
There was a problem hiding this 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.
| - 'version_spec.rb' | ||
|
|
||
| # Offense count: 76 | ||
| # Offense count: 81 |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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
describethat encompasses the entire file for each of these. I don't think it would cause much additional clutter at least to thecmd/*,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
describeso 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 👍🏻
brew stylewith your changes locally?brew typecheckwith your changes locally?brew testswith your changes locally?brew manlocally and committed any changes?This PR adds a
brew completionscommand and makes linking completions opt-in only per discussion in #10229.Once this PR is merged, users will see a message in
brew updateinforming them that completions are no longer linked. It will direct them to runbrew completions linkto link completions. The user can choose whether completions are linked by runningbrew completions linkorbrew completions unlink. They can check whether completions are linked by runningbrew completionsorbrew completions status.CC @MikeMcQuaid