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

audit: Port audit_conflicts method to rubocop and add tests #2843

Merged
merged 1 commit into from Jul 8, 2017

Conversation

GauthamGoli
Copy link
Contributor

@GauthamGoli GauthamGoli commented Jun 30, 2017

  • Have you followed the guidelines in our Contributing document?
  • Have you checked to ensure there aren't other open Pull 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 run brew tests with your changes locally?

#569
This PR partially ports audit_conflicts method to a custom cop. Remaining code in audit_conflicts uses Homebrew internals methods and I could not figure out their exact functionality to port them. Please let me know if this approach is not good.

Copy link
Member

@MikeMcQuaid MikeMcQuaid left a comment

Choose a reason for hiding this comment

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

Two tiny nits but otherwise looking good 👍

# This cop audits versioned Formulae for `conflicts_with`
class Conflicts < FormulaCop
MSG = <<-EOS.undent
Versioned formulae should not use `conflicts_with`.
Copy link
Member

Choose a reason for hiding this comment

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

Only indent by two characters here.


def audit_formula(_node, _class_node, _parent_class_node, body)
return unless versioned_formula?
whitelist = %w[node@ bash-completion@].freeze
Copy link
Member

Choose a reason for hiding this comment

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

Let's make this a constant earlier in the class and have one formula per line so they stand out a bit more

@GauthamGoli GauthamGoli force-pushed the audit_conflicts_rubocop branch 2 times, most recently from 8a9f570 to 0db695e Compare July 3, 2017 04:09
EOS

WHITELIST = %w[node@
bash-completion@].freeze
Copy link
Member

Choose a reason for hiding this comment

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

Let's go for:

WHITELIST = %w[
   node@
   bash-completion@
].freeze

Copy link
Member

Choose a reason for hiding this comment

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

that will reduce the diff on future additions here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Waoh. Never thought of that!

@GauthamGoli
Copy link
Contributor Author

GauthamGoli commented Jul 7, 2017

@MikeMcQuaid I'm moving the two private methods defined in conflicts_cop.rb to formula_cop.rb as they can be reused in other cops. ( options_cop.rb)

Copy link
Contributor

@JCount JCount left a comment

Choose a reason for hiding this comment

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

Looks good to me. 👍

@MikeMcQuaid MikeMcQuaid merged commit f1fa475 into Homebrew:master Jul 8, 2017
@Homebrew Homebrew locked and limited conversation to collaborators May 4, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants