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.

Sign up for GitHub

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

Jump to bottom

audit: whitelist bash-completion@* to use conflicts_with #2269

Merged
merged 3 commits into from
Mar 6, 2017
Merged

audit: whitelist bash-completion@* to use conflicts_with #2269

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
4e50562
audit: whitelist bash-completion@* to use conflicts_with
JCount Mar 5, 2017
460c4a3
audit: deconstruct versioned_conflicts_whitelist using splat
JCount Mar 5, 2017
3db3b08
audit: freeze versioned_conflicts_whitelist
JCount Mar 5, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
4 changes: 3 additions & 1 deletion Library/Homebrew/dev-cmd/audit.rb
View file
Open in desktop
Expand Up @@ -532,8 +532,10 @@ def audit_conflicts
end
end

versioned_conflicts_whitelist = %w[node@ bash-completion@].freeze
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is there any compelling reason to move this out of the audit_conflicts method and turn it into a true constant?

Copy link
Member

Choose a reason for hiding this comment

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

I think it would make sense to move all of these whitelists outside, since they really are supposed to be constants, and it makes the methods easier to read if there aren't a billion lines of constants in-between “actual“ code.

Copy link
Contributor Author

@JCount JCount Mar 5, 2017 •
edited

Choose a reason for hiding this comment

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

That sounds like a perfectly valid argument to me. When I get a chance, I will transform this into a true constant. I think the other lists should probably be done in a separate PR.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, I'd leave this as is for now, and convert all of them in one go in a separate PR.


return unless formula.conflicts.any? && formula.versioned_formula?
return if formula.name.start_with? "node@"
return if formula.name.start_with?(*versioned_conflicts_whitelist)
problem <<-EOS
Versioned formulae should not use `conflicts_with`.
Use `keg_only :versioned_formula` instead.
Expand Down