Conversation
- Fixes #21732. - When `brew bundle remove`ing, also clean up the directly preceding description comment (from `brew bundle dump --describe`, or any others).
issyl0
force-pushed
the
bundle-remove-comments
branch
from
4bdffd1 to
1974d89
Compare
There was a problem hiding this comment.
Pull request overview
This PR updates brew bundle remove so that when an entry is removed from a Brewfile, any directly preceding description comment (typically produced by brew bundle dump --describe) is also cleaned up, addressing #21732.
Changes:
- Update the Bundle remover logic to drop a directly preceding comment line when removing a matching entry.
- Add an RSpec example asserting that a preceding description comment is removed along with the entry.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| Library/Homebrew/bundle/remover.rb | Adjusts line-removal logic to also remove a preceding comment line when an entry is removed. |
| Library/Homebrew/test/bundle/commands/remove_spec.rb | Adds coverage to ensure a preceding description comment is removed with the entry. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| let(:content) do | ||
| <<~BREWFILE | ||
| # Program providing model for GNU coding standards and practices | ||
| brew "hello" | ||
| # Get a file from an HTTP, HTTPS or FTP server | ||
| brew "curl" | ||
| BREWFILE | ||
| end | ||
|
|
||
| it "removes both the entry and its description comment" do | ||
| expect { remove }.not_to raise_error | ||
|
|
||
| expect(File.read(file)).to eq <<~BREWFILE | ||
| # Get a file from an HTTP, HTTPS or FTP server | ||
| brew "curl" | ||
| BREWFILE |
There was a problem hiding this comment.
Is that a thing? I don't think that's a thing - descriptions are short?
There was a problem hiding this comment.
It's perhaps a thing in theory. Check homebrew/core and homebrew/cask. Might be worth adding an audit/rubocop for this afterwards if not.
MikeMcQuaid
left a comment
MikeMcQuaid
left a comment
There was a problem hiding this comment.
Thanks, looking good so far!
| content.split("\n").each do |line| | ||
| if line&.match?(entry_regex) |
There was a problem hiding this comment.
| content.split("\n").each do |line| | |
| if line&.match?(entry_regex) | |
| content.split("\n").compact.each do |line| | |
| if line.match?(entry_regex) |
| new_lines << line | ||
| end | ||
| end | ||
|
|
||
| new_content = "#{new_lines.join("\n")}\n" |
There was a problem hiding this comment.
.filter_map might be nicer here
| let(:content) do | ||
| <<~BREWFILE | ||
| # Program providing model for GNU coding standards and practices | ||
| brew "hello" | ||
| # Get a file from an HTTP, HTTPS or FTP server | ||
| brew "curl" | ||
| BREWFILE | ||
| end | ||
|
|
||
| it "removes both the entry and its description comment" do | ||
| expect { remove }.not_to raise_error | ||
|
|
||
| expect(File.read(file)).to eq <<~BREWFILE | ||
| # Get a file from an HTTP, HTTPS or FTP server | ||
| brew "curl" | ||
| BREWFILE |
There was a problem hiding this comment.
It's perhaps a thing in theory. Check homebrew/core and homebrew/cask. Might be worth adding an audit/rubocop for this afterwards if not.
|
|
||
| content.split("\n").each do |line| | ||
| if line&.match?(entry_regex) | ||
| # Remove any directly preceding comment - it could be a description |
There was a problem hiding this comment.
It also could not be 😁. Worth directly comparing with the description from both the tab and the current formula/cask/JSON I think.
brew lgtm(style, typechecking and tests) with your changes locally?brew bundle removedoes not clean up descriptions in Brewfile #21732.brew bundle removeing, also clean up the directly preceding description comment (frombrew bundle dump --describe, or any others).