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: audit_components method to rubocops and tests #2465

Merged
merged 1 commit into from Apr 24, 2017

Conversation

Projects
None yet
2 participants
@GauthamGoli
Copy link
Member

GauthamGoli commented Apr 8, 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?

This is in continuation of the PR #2242 and issue #569 .
This PR ports audit_components https://github.com/Homebrew/brew/blob/master/Library/Homebrew/dev-cmd/audit.rb#L251-L309
method to custom cops.

@GauthamGoli GauthamGoli changed the title Port audit_components method to rubocops and add corresponding tests audit: audit_components method to rubocops and tests Apr 8, 2017

@GauthamGoli GauthamGoli force-pushed the GauthamGoli:audit_components_port_rubocop branch from 1e22189 to ce4491f Apr 10, 2017

@MikeMcQuaid
Copy link
Member

MikeMcQuaid left a comment

Looking great so far 👍

class FormulaComponentsOrder < FormulaCop
def audit_formula(_node, _class_node, _parent_class_node, formula_class_body_node)
component_precedence_list = [
[{ "name" => :include, "type" => "method" }],

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Apr 20, 2017

Member

Random comment that symbols for both keys and values here might be nice if it's not overly complex? Also, what about method_call vs. method_definition vs. block_call or something?

This comment has been minimized.

@GauthamGoli

GauthamGoli Apr 21, 2017

Member

method_call, block_call and method_definition nodes are of type method, block and def respectively in Rubocop. Do you mean, we should have something more explicit for type values in component_precedence_list?

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Apr 21, 2017

Member

Oh, that's fine then, wasn't clear they were internal types. Still think symbols (at least for keys) would be great.

This comment has been minimized.

@GauthamGoli

GauthamGoli Apr 22, 2017

Member

I updated them with symbols and also used method_call ,block_call and method_definition instead, as we want to hide the rubocop specific details in formula_cop.rb itself.

@@ -74,6 +102,35 @@ def method_called?(node, method_name)
false
end

def method_called?(node, method_name)
# Check if method_name is called among the direct children nodes in the given node

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Apr 20, 2017

Member

I'd put these comments above rather than below the method definitions.

# Checks for precedence, returns the first pair of precedence violating nodes
next_nodes.each do |each_next_node|
first_nodes.each do |each_first_node|
return [each_first_node, each_next_node] if component_precedes?(each_first_node, each_next_node)

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Apr 20, 2017

Member

Generally lines over 80 characters (or thereabouts) are to be avoided when possible.


def component_precedes?(first_node, next_node)
# If first node does not precede next_node, sets appropriate instance variables for reporting
return false unless line_number(first_node) > line_number(next_node)

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Apr 20, 2017

Member

Flip the > to make it an if

@GauthamGoli GauthamGoli force-pushed the GauthamGoli:audit_components_port_rubocop branch from ce4491f to 413a7e5 Apr 22, 2017

@MikeMcQuaid MikeMcQuaid merged commit ceb1629 into Homebrew:master Apr 24, 2017

3 checks passed

codecov/patch 97.46% of diff hit (target 64.03%)
Details
codecov/project 64.08% (+0.05%) compared to cd59249
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@MikeMcQuaid

This comment has been minimized.

Copy link
Member

MikeMcQuaid commented Apr 24, 2017

Thanks again @GauthamGoli!

@GauthamGoli

This comment has been minimized.

Copy link
Member

GauthamGoli commented Apr 24, 2017

@MikeMcQuaid The violating block/method/ def s in the editor are highlighted at their names. Do we want the whole block of code to get highlighted instead?

@MikeMcQuaid

This comment has been minimized.

Copy link
Member

MikeMcQuaid commented Apr 24, 2017

@GauthamGoli My editor doesn't support the highlighting so I think the current version is fine, personally. Up to you, though!

@Homebrew Homebrew locked and limited conversation to collaborators May 3, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.