Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

formula: allow using modern bash completion dir#21254

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Open
cho-m wants to merge1 commit intomain
base:main
Choose a base branch
Loading
frombash4-completions

Conversation

@cho-m
Copy link
Member

@cho-mcho-m commentedDec 15, 2025
edited
Loading

  • Have you followed the guidelines in ourContributing document?
  • Have you checked to ensure there aren't other openPull 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 runbrew lgtm (style, typechecking and tests) with your changes locally?

I was previously thinking of adding a separate method:

But 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 allbash_completion.install usage remains the same.


This is needed as we recently enabled Click Bash completions

Which are incompatible with system Bash


One scenario that this does not automatically support is Bash 4 withoutbash-completion@2 installed. For now, users will need to manually adjust their Bash config file to load these.

We could adjusthttps://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 installbash-completion@2.

This directory can be used for Bash completions that need Bash >= 4 orbash-completion >= 2. It is automatically searched by bash-completion.Also make `:click` completions install into that directory.
CopilotAI review requested due to automatic review settingsDecember 15, 2025 19:45
Copy link
Contributor

CopilotAI left a comment

Choose a reason for hiding this comment

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

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:

  • Modifiedbash_completion method to accept an optionalcompat parameter (default:true)
  • Updatedgenerate_completions_from_executable to 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.

FileDescription
Library/Homebrew/formula.rbAddedcompat parameter tobash_completion method and integrated it withgenerate_completions_from_executable to support modern Bash completion directory
Library/Homebrew/test/formula_spec.rbAdded 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.

Copy link
Member

@MikeMcQuaidMikeMcQuaid left a comment

Choose a reason for hiding this comment

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

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,
Copy link
Member

Choose a reason for hiding this comment

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

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)}
defbash_completion=prefix/"etc/bash_completion.d"
sig{params(compat:T::Boolean).returns(Pathname)}
defbash_completion(compat:true)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
defbash_completion(compat:true)
defbash_completion(bash3:false)

maybe?

Copy link
MemberAuthor

@cho-mcho-mDec 16, 2025
edited
Loading

Choose a reason for hiding this comment

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

I had went withcompat due to bash-completion naming, though I'm open to alternatives .

pkgconf bash-completion --variable compatdir --define-prefix/opt/homebrew/etc/bash_completion.dpkgconf bash-completion --variable completionsdir --define-prefix/opt/homebrew/share/bash-completion/completions

Some other ideas I was considering:

  • bash_completion(use_compatdir: false)
  • bash_completion(version = :v1), sobash_completion(:v2).install ...

If usingbash3, it should also default totrue to preserve existing behavior.

Copy link
Member

Choose a reason for hiding this comment

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

  • bash_completion(use_compatdir: false)
  • bash_completion(version = :v1), sobash_completion(:v2).install ...

Either of these are preferable to me; not obvious from reading whatcompat means.

If usingbash3, it should also default totrue to preserve existing behavior.

Agreed.

Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@MikeMcQuaidMikeMcQuaidMikeMcQuaid approved these changes

Copilot code reviewCopilotCopilot left review comments

At least 1 approving review is required to merge this pull request.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@cho-m@MikeMcQuaid

[8]ページ先頭

©2009-2025 Movatter.jp