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_text method to rubocop and add tests #2662

Merged
merged 4 commits into from May 30, 2017

Conversation

Projects
None yet
3 participants
@GauthamGoli
Copy link
Member

GauthamGoli commented May 21, 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
I ported most of the code in audit_text method except few lines which use Homebrew's internal code.

@GauthamGoli GauthamGoli force-pushed the GauthamGoli:audit_text_rubocop branch 3 times, most recently from ac5b439 to c089d7e May 21, 2017

def depends_on?(node, dependency_name)
dependency_nodes = find_every_method_call_by_name(node, :depends_on)
idx = dependency_nodes.index do |n|
depends_on_name_type?(n, dependency_name, :required) ||

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid May 25, 2017

Member

I'd probably use next, continue or break here rather than chaining ||

depends_on_name_type?(n, dependency_name, :recommended) ||
depends_on_name_type?(n, dependency_name, :run)
end
return nil if idx.nil?

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid May 25, 2017

Member

return if idx.nil?

when :required
dependency_type_match = !node.method_args.nil? && node.method_args.first.str_type?
dependency_name_match = (string_content(node.method_args.first) == dependency_name) if dependency_type_match
when :build || :optional || :recommended || :run

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid May 25, 2017

Member

I'd consider using , rather than ||

when :build || :optional || :recommended || :run
dependency_type_match = !node.method_args.nil? &&
node.method_args.first.hash_type? &&
node.method_args.first.values[0].children.first == dependency_type

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid May 25, 2017

Member

values.first

dependency_type_match = !node.method_args.nil? &&
node.method_args.first.hash_type? &&
node.method_args.first.values[0].children.first == dependency_type
dependency_name_match = (node.method_args.first.keys[0].children.first == dependency_name) if dependency_type_match

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid May 25, 2017

Member

keys.first and use a block if to slim the line.

case dependency_type
when :required
dependency_type_match = !node.method_args.nil? && node.method_args.first.str_type?
dependency_name_match = (string_content(node.method_args.first) == dependency_name) if dependency_type_match

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid May 25, 2017

Member

Use a block if to slim the line

problem "Please set plist_options when using a formula-defined plist."
end

if depends_on?(body_node, "openssl") && depends_on?(body_node, "libressl")

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid May 25, 2017

Member

Ideally body_node wouldn't need to be passed through here.

factory_calls = find_every_method_call_by_name(body_node, :factory)
unless factory_calls.nil?
factory_calls.each do |m|
if !m.children.empty? && m.children[0] && string_content(m.children[0]) == "Formula"

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid May 25, 2017

Member

children.first

@MikeMcQuaid

This comment has been minimized.

Copy link
Member

MikeMcQuaid commented May 25, 2017

@GauthamGoli I'm liking the direction here. It'd be cool to think if there's ways of slimming down the text_cop.rb logic even further so it more closely resembles the audit. My only concern is that it may be harder to write new cops than new audits.

@GauthamGoli GauthamGoli force-pushed the GauthamGoli:audit_text_rubocop branch 3 times, most recently from fe09bc9 to 654a139 May 29, 2017

@GauthamGoli

This comment has been minimized.

Copy link
Member

GauthamGoli commented May 30, 2017

I've added few more generalized methods in formula_cop.rb to consolidate text_cop.rb, please let me know your thoughts if to go ahead with this or revert.

@GauthamGoli GauthamGoli force-pushed the GauthamGoli:audit_text_rubocop branch from 654a139 to cfbdc17 May 30, 2017

@MikeMcQuaid

This comment has been minimized.

Copy link
Member

MikeMcQuaid commented May 30, 2017

Great work here @GauthamGoli!

@MikeMcQuaid MikeMcQuaid merged commit 344c492 into Homebrew:master May 30, 2017

3 checks passed

codecov/patch Coverage not affected when comparing d1f802c...cfbdc17
Details
codecov/project 64.41% (+<.01%) compared to d1f802c
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@JCount

This comment has been minimized.

Copy link
Contributor

JCount commented May 30, 2017

@GauthamGoli Good job!

@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.