formula: allow using modern bash completion dir #21254
Conversation
This directory can be used for Bash completions that need Bash >= 4 or bash-completion >= 2. It is automatically searched by bash-completion. Also make `:click` completions install into that directory.
There was a problem hiding this comment.
Pull request overview
This PR adds support for installing Bash completions to the modern XDG-based directory (share/bash-completion/completions) in addition to the legacy compatibility directory (etc/bash_completion.d). This is needed to support Click-based Bash completions which require Bash >= 4 or bash-completion >= 2, incompatible with the system Bash 3 on macOS.
Key changes:
- Modified
bash_completionmethod to accept an optionalcompatparameter (default:true) - Updated
generate_completions_from_executableto automatically use the modern directory for Click-formatted completions - Added test coverage for the new functionality
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| Library/Homebrew/formula.rb | Added compat parameter to bash_completion method and integrated it with generate_completions_from_executable to support modern Bash completion directory |
| Library/Homebrew/test/formula_spec.rb | Added test case to verify Click completions are installed to the modern directory while regular completions remain in the compat directory |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
MikeMcQuaid
left a comment
MikeMcQuaid
left a comment
There was a problem hiding this comment.
Thanks, looks good! A few discussions around style/naming.
|
|
||
| completion_script_path_map = { | ||
| bash: bash_completion/base_name, | ||
| bash: bash_completion(compat: shell_parameter_format != :click)/base_name, |
There was a problem hiding this comment.
Pulling this into a variable with a comment would be nice; not clear why this is the case and a little hard to read the logic inline the method call
| sig { returns(Pathname) } | ||
| def bash_completion = prefix/"etc/bash_completion.d" | ||
| sig { params(compat: T::Boolean).returns(Pathname) } | ||
| def bash_completion(compat: true) |
There was a problem hiding this comment.
| def bash_completion(compat: true) | |
| def bash_completion(bash3: false) |
maybe?
There was a problem hiding this comment.
I had went with compat due to bash-completion naming, though I'm open to alternatives .
❯ pkgconf bash-completion --variable compatdir --define-prefix
/opt/homebrew/etc/bash_completion.d
❯ pkgconf bash-completion --variable completionsdir --define-prefix
/opt/homebrew/share/bash-completion/completionsSome other ideas I was considering:
bash_completion(use_compatdir: false)bash_completion(version = :v1), sobash_completion(:v2).install ...
If using bash3, it should also default to true to preserve existing behavior.
brew lgtm(style, typechecking and tests) with your changes locally?I was previously thinking of adding a separate method:
bash-completion@2completions #14270But I wasn't happy on naming as version number doesn't make much sense given the path will likely remain for v3, v4, etc as it is based on XDG.
This PR is a different idea for DSL of using an argument, with default to keep using compatdir path so all
bash_completion.installusage remains the same.This is needed as we recently enabled Click Bash completions
:clickshell completions be enabled for Bash? #20188Which are incompatible with system Bash
One scenario that this does not automatically support is Bash 4 without
bash-completion@2installed. For now, users will need to manually adjust their Bash config file to load these.We could adjust https://docs.brew.sh/Shell-Completion#configuring-completions-in-bash but it will make the logic more complicated to detect which Bash version is running and add multiple directories to load from.
Easier response to this scenario is to tell users to install
bash-completion@2.