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_homepage method to rubocop and add tests #2631

Merged
merged 1 commit into from May 15, 2017

Conversation

Projects
None yet
2 participants
@GauthamGoli
Copy link
Member

GauthamGoli commented May 14, 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?

Ports all the rules defined in audit_homepage method except for the ones that execute with --online option , they will be placed in a separate cop category for online audit rules.

@GauthamGoli GauthamGoli changed the title Port audit_homepage method to rubocop and add tests audit: Port audit_homepage method to rubocop and add tests May 14, 2017

homepage = homepage_node.nil? ? "" : string_content(parameters(homepage_node).first)

problem "Formula should have a homepage." if homepage_node.nil? || homepage.empty?
problem "The homepage should start with http or https (URL is #{homepage})." unless homepage =~ %r{^https?://}

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid May 14, 2017

Member

This line is pretty long so I'd stick the unless onto the previous line.

class Homepage < FormulaCop
def audit_formula(_node, _class_node, _parent_class_node, formula_class_body_node)
homepage_node = find_node_method_by_name(formula_class_body_node, :homepage)
homepage = homepage_node.nil? ? "" : string_content(parameters(homepage_node).first)

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid May 14, 2017

Member

I'd do something like:

homepage = if homepage_node
  string_content(parameters(homepage_node).first) 
else
  ""
end

# Check for http:// GitHub homepage urls, https:// is preferred.
# Note: only check homepages that are repo pages, not *.github.com hosts
problem "Please use https:// for #{homepage}" if homepage.start_with? "http://github.com/"

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid May 14, 2017

Member

I wonder if it's worth turning this and everything below into a big case statement? What do you think?

This comment has been minimized.

@GauthamGoli

GauthamGoli May 14, 2017

Member

Yeah, I think its better that way.

@GauthamGoli GauthamGoli force-pushed the GauthamGoli:audit_homepage_rubocop branch from 11d6682 to 91efcb0 May 14, 2017

@MikeMcQuaid MikeMcQuaid merged commit ea8be17 into Homebrew:master May 15, 2017

3 checks passed

codecov/patch 96% of diff hit (target 64.38%)
Details
codecov/project Absolute coverage decreased by -0.04% but relative coverage increased by +31.61% compared to e0ab162
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@MikeMcQuaid

This comment has been minimized.

Copy link
Member

MikeMcQuaid commented May 15, 2017

Great work again @GauthamGoli!

@GauthamGoli

This comment has been minimized.

Copy link
Member

GauthamGoli commented May 15, 2017

Thanks for the review! :)

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