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

Add --describe arg to brew add #14

Merged

Conversation

ian-h-chamberlain
Copy link
Contributor

Hi, I hope it's okay I didn't file an issue before implementing this! It was pretty simple to write, so I thought I'd just go ahead and open a PR.

This adds a --describe to insert a comment line with a description when brew adding a cask/formula. It also respects the existing HOMEBREW_BUNDLE_DUMP_DESCRIBE environment variable that has the same effect for brew bundle dump.

@superatomic superatomic self-requested a review December 23, 2023 18:35
@superatomic superatomic added the enhancement New feature or request label Dec 23, 2023
@superatomic
Copy link
Owner

Thank you for developing this feature! I would love to add this to brew add. Before this can be merged, there's a few things that need to be done.

  • This project is actually duel-licensed under the BSD-2-Clause License and the MIT License as part of the efforts to merge this tap with homebrew/homebrew-bundle as a core feature (see brew bundle add && brew bundle remove Homebrew/homebrew-bundle#818). I had forgotten to update the licensing information in this repository to reflect that, which I have now rectified in Add MIT License #15. Of course, since you submitted this PR before I clarified the change, I need your confirmation that you'd be willing to license your PR under both licenses.
  • I'd like to modify brew drop to remove the comments generated by --describe, so adding and removing a formula doesn't result in leftover comments persisting. I am willing to implement this myself, so no action is required on your part, but if you want to try to implement it yourself, you are more than welcome to do so. Changing brew drop in this way will probably require a significant rework of the current code to be able to remove lines that have already been checked.

Thanks again for this PR!

@ian-h-chamberlain
Copy link
Contributor Author

I need your confirmation that you'd be willing to license your PR under both licenses.

Absolutely, this contribution can be licensed under both MIT and BSD-2-Clause. No concerns there, but thanks for checking.

I'd like to modify brew drop to remove the comments generated by --describe, so adding and removing a formula doesn't result in leftover comments persisting.

This makes sense and wasn't something I considered at first. I think I have a basic implementation working without too much hassle, so I'll go ahead and push it — but if you'd like to have something more robust in place then I'm happy to defer to your implementation as well.

Thanks for taking the time to look at this!

@superatomic
Copy link
Owner

Thank you! I have a few ideas for how to remove the description that I would like to try myself. A potential problem with your current implementation is that it breaks if the formula description itself is ever changed (or if the user modifies the comment, like you've described). My current idea that I'm planning to implement is for it to remove all comments that come directly before the line until it hits a line that contains either whitespace or a non-comment line (so dropwhile /^# / matches). My concern is that it could possibly remove too much, depending on how the user structures their Brewfile, but I can't think of that many situations where this would occur.

I'll get around to implementing this soon.

@ian-h-chamberlain
Copy link
Contributor Author

Fair enough, it was a pretty naive attempt and I was guessing you had planned something a little more robust like you describe. Would you prefer I undo the changes I made to try and merge add --describe sooner, or wait for your planned implementation? If you'd rather merge it sooner I'm happy to update my branch, but no rush either way.

cmd/add.rb Outdated Show resolved Hide resolved
@superatomic
Copy link
Owner

I think it's probably best to implement both together.

@superatomic
Copy link
Owner

superatomic commented Dec 26, 2023

I've made some changes to the code and implemented the logic for brew drop. It looks ready to merge to me, but I'd love to hear your thoughts first before I do.

File.foreach(brewfile) do |line|
unless line.match(/^\s*#{brewfile_prefix_type}\s+["'](#{regex_name})["']/)
lines.push(line)
if line.match(/^\s*# (?!brew|cask|tap|mas|whalebrew|vscode)\w/)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just curious, why bother with this ?! match here? To avoid capturing places where someone commented out a line like # brew "foo" ?

Copy link
Owner

Choose a reason for hiding this comment

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

Yes, that's the goal. It's a pretty ugly hack but I think it'll address the most common cases where a comment would otherwise incorrectly be removed.

Copy link
Contributor Author

@ian-h-chamberlain ian-h-chamberlain left a comment

Choose a reason for hiding this comment

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

LGTM! Not sure about exactly what cases you want to cover with drop but the logic as written seems like it would cover what was added by --describe.

@superatomic superatomic merged commit 991269c into superatomic:main Dec 27, 2023
@ian-h-chamberlain ian-h-chamberlain deleted the feature/add-with-describe branch December 27, 2023 02:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants