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_urls partially to rubocop and add corresponding tests #2911

Merged
merged 1 commit into from Jul 25, 2017

Conversation

Projects
None yet
3 participants
@GauthamGoli
Copy link
Member

GauthamGoli commented Jul 18, 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 attempts to partially port audit_urls method of ResourceAuditor to RuboCop custom cop and adds corresponding tests.
I tried to reuse regex_match_group method of FormulaCop. Instead, for the sake of brevity of code in this cop, string_content method can be used to extract string part from url node and a case statement to match regexs.

Edit: I used a closure to reduce redundant code, please let me know what you think!

@GauthamGoli GauthamGoli force-pushed the GauthamGoli:audit_urls_rubocop_part_1 branch 2 times, most recently from 10f0055 to 77bfdaa Jul 19, 2017


# Check a variety of SSL/TLS URLs that don't consistently auto-redirect
# or are overly common errors that need to be reduced & fixed over time.
http_urls_pattern = Regexp.union([%r{^http://ftp\.gnu\.org/},

This comment has been minimized.

@JCount

JCount Jul 20, 2017

Contributor

Perhaps this could use a name other than http_urls_pattern? One that better reflects the fact that the urls which match this pattern should be changed to https:// might be a better fit . https_url_id_pattern is a bad substitute, and but it would probably be good to make this rely a little less on the inline comment. However, if nothing better comes to mind this is still fine. 😄

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Jul 21, 2017

Member

http_to_https_patterns

@MikeMcQuaid
Copy link
Member

MikeMcQuaid left a comment

A few nits but nothing major, nice work again!


# Check a variety of SSL/TLS URLs that don't consistently auto-redirect
# or are overly common errors that need to be reduced & fixed over time.
http_urls_pattern = Regexp.union([%r{^http://ftp\.gnu\.org/},

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Jul 21, 2017

Member

http_to_https_patterns

%r{^http://(?:[^/]*\.)?freedesktop\.org},
%r{^http://(?:[^/]*\.)?mirrorservice\.org/}])

# CPAN pattern

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Jul 21, 2017

Member

Don't need comments like this; inferred from the variable name.

urls = find_every_method_call_by_name(body_node, :url)
mirrors = find_every_method_call_by_name(body_node, :mirror)

audit_urls(urls, gnu_pattern) do |match_obj, url|

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Jul 21, 2017

Member

do |match, url|

audit_urls(mirrors, /.*/) do |_, mirror|
urls.each do |url|
url_string = string_content(parameters(url).first)
if url_string.eql?(mirror)

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Jul 21, 2017

Member

next unless url_string.eql?(mirror)

problem "Please use \"https://ftp.gnu.org/gnu/#{match_obj[1]}\" instead of #{url}."
end

audit_urls(urls, fossies_pattern) do |_, _|

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Jul 21, 2017

Member

I'm not sure but you may be able to omit |_, _| entirely.

end

audit_urls(urls, gnome_pattern) do |match_obj, url|
problem "#{url} should be `https://download.gnome.org/#{match_obj[2]}`"

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Jul 21, 2017

Member

Let's define e.g. gnome_pattern immediately above where it is called so it's obvious what regex group match[2] refers to.

This comment has been minimized.

@GauthamGoli

GauthamGoli Jul 21, 2017

Member

Yeah. Makes more sense.

it "with offenses" do
formulas = [{
"url" => "https://ftpmirror.gnu.org/lightning/lightning-2.1.0.tar.gz",
"msg" => "Please use \"https://ftp.gnu.org/gnu/lightning/lightning-2.1.0.tar.gz\" instead of https://ftpmirror.gnu.org/lightning/lightning-2.1.0.tar.gz.",

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Jul 21, 2017

Member

Use ' where the string includes " but doesn't need interpolation.

@GauthamGoli GauthamGoli force-pushed the GauthamGoli:audit_urls_rubocop_part_1 branch from 77bfdaa to b7ddd27 Jul 21, 2017

@GauthamGoli

This comment has been minimized.

Copy link
Member

GauthamGoli commented Jul 21, 2017

@MikeMcQuaid Pushed the changes and squashed the two commits into one.

@JCount

JCount approved these changes Jul 21, 2017

Copy link
Contributor

JCount left a comment

The changes look good to me, but I will of course let MikeMcQuaid make the final call.

@MikeMcQuaid MikeMcQuaid merged commit 7041f7e into Homebrew:master Jul 25, 2017

3 checks passed

codecov/patch 87.8% of diff hit (target 65.78%)
Details
codecov/project 65.86% (+0.08%) compared to 9747bc3
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@MikeMcQuaid

This comment has been minimized.

Copy link
Member

MikeMcQuaid commented Jul 25, 2017

Thanks again @GauthamGoli!

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