Refactor legacy_patches_cop to merge external patch audit rules in a single cop and add tests
This commit is contained in:
Gautham Goli
parent
43cec10000
commit
66ffdb5b06
@ -6,7 +6,7 @@ require_relative "./rubocops/homepage_cop"
|
||||
require_relative "./rubocops/text_cop"
|
||||
require_relative "./rubocops/caveats_cop"
|
||||
require_relative "./rubocops/checksum_cop"
|
||||
require_relative "./rubocops/legacy_patches_cop"
|
||||
require_relative "./rubocops/patches_cop"
|
||||
require_relative "./rubocops/conflicts_cop"
|
||||
require_relative "./rubocops/options_cop"
|
||||
require_relative "./rubocops/urls_cop"
|
||||
|
||||
@ -380,40 +380,6 @@ module RuboCop
|
||||
add_offense(@offensive_node, @offense_source_range, msg)
|
||||
end
|
||||
|
||||
def patch_problems(patch)
|
||||
patch_url = string_content(patch)
|
||||
gh_patch_patterns = Regexp.union([%r{/raw\.github\.com/},
|
||||
%r{gist\.github\.com/raw},
|
||||
%r{gist\.github\.com/.+/raw},
|
||||
%r{gist\.githubusercontent\.com/.+/raw}])
|
||||
if regex_match_group(patch, gh_patch_patterns)
|
||||
unless patch_url =~ /[a-fA-F0-9]{40}/
|
||||
problem "GitHub/Gist patches should specify a revision:\n#{patch_url}"
|
||||
end
|
||||
end
|
||||
|
||||
gh_patch_diff_pattern = %r{https?://patch-diff\.githubusercontent\.com/raw/(.+)/(.+)/pull/(.+)\.(?:diff|patch)}
|
||||
if match_obj = regex_match_group(patch, gh_patch_diff_pattern)
|
||||
problem <<-EOS.undent
|
||||
use GitHub pull request URLs:
|
||||
https://github.com/#{match_obj[1]}/#{match_obj[2]}/pull/#{match_ojb[3]}.patch
|
||||
Rather than patch-diff:
|
||||
#{patch_url}
|
||||
EOS
|
||||
end
|
||||
|
||||
if regex_match_group(patch, %r{macports/trunk})
|
||||
problem "MacPorts patches should specify a revision instead of trunk:\n#{patch_url}"
|
||||
end
|
||||
|
||||
if regex_match_group(patch, %r{^http://trac\.macports\.org})
|
||||
problem "Patches from MacPorts Trac should be https://, not http:\n#{patch_url}"
|
||||
end
|
||||
|
||||
return unless regex_match_group(patch, %r{^http://bugs\.debian\.org})
|
||||
problem "Patches from Debian should be https://, not http:\n#{patch_url}"
|
||||
end
|
||||
|
||||
private
|
||||
|
||||
def formula_class?(node)
|
||||
|
||||
@ -1,30 +0,0 @@
|
||||
require_relative "./extend/formula_cop"
|
||||
require_relative "../extend/string"
|
||||
|
||||
module RuboCop
|
||||
module Cop
|
||||
module FormulaAudit
|
||||
# This cop checks for and audits legacy patches in Formulae
|
||||
class LegacyPatches < FormulaCop
|
||||
def audit_formula(_node, _class_node, _parent_class_node, body)
|
||||
patches_node = find_method_def(body, :patches)
|
||||
return if patches_node.nil?
|
||||
legacy_patches = find_strings(patches_node)
|
||||
problem "Use the patch DSL instead of defining a 'patches' method"
|
||||
legacy_patches.each { |p| patch_problems(p) }
|
||||
end
|
||||
end
|
||||
|
||||
class ExternalPatches < FormulaCop
|
||||
def audit_formula(_node, _class_node, _parent_class_node, body)
|
||||
patches = find_all_blocks(body, :patch)
|
||||
patches.each do |patch_block|
|
||||
url_node = find_node_method_by_name(patch_block, :url)
|
||||
url_string = parameters(url_node).first
|
||||
patch_problems(url_string)
|
||||
end
|
||||
end
|
||||
end
|
||||
end
|
||||
end
|
||||
end
|
||||
62
Library/Homebrew/rubocops/patches_cop.rb
Normal file
62
Library/Homebrew/rubocops/patches_cop.rb
Normal file
@ -0,0 +1,62 @@
|
||||
require_relative "./extend/formula_cop"
|
||||
require_relative "../extend/string"
|
||||
|
||||
module RuboCop
|
||||
module Cop
|
||||
module FormulaAudit
|
||||
# This cop audits patches in Formulae
|
||||
class Patches < FormulaCop
|
||||
def audit_formula(_node, _class_node, _parent_class_node, body)
|
||||
external_patches = find_all_blocks(body, :patch)
|
||||
external_patches.each do |patch_block|
|
||||
url_node = find_every_method_call_by_name(patch_block, :url).first
|
||||
url_string = parameters(url_node).first
|
||||
patch_problems(url_string)
|
||||
end
|
||||
|
||||
patches_node = find_method_def(body, :patches)
|
||||
return if patches_node.nil?
|
||||
legacy_patches = find_strings(patches_node)
|
||||
problem "Use the patch DSL instead of defining a 'patches' method"
|
||||
legacy_patches.each { |p| patch_problems(p) }
|
||||
end
|
||||
|
||||
private
|
||||
|
||||
def patch_problems(patch)
|
||||
patch_url = string_content(patch)
|
||||
gh_patch_patterns = Regexp.union([%r{/raw\.github\.com/},
|
||||
%r{gist\.github\.com/raw},
|
||||
%r{gist\.github\.com/.+/raw},
|
||||
%r{gist\.githubusercontent\.com/.+/raw}])
|
||||
if regex_match_group(patch, gh_patch_patterns)
|
||||
unless patch_url =~ /[a-fA-F0-9]{40}/
|
||||
problem "GitHub/Gist patches should specify a revision:\n#{patch_url}"
|
||||
end
|
||||
end
|
||||
|
||||
gh_patch_diff_pattern = %r{https?://patch-diff\.githubusercontent\.com/raw/(.+)/(.+)/pull/(.+)\.(?:diff|patch)}
|
||||
if match_obj = regex_match_group(patch, gh_patch_diff_pattern)
|
||||
problem <<-EOS.undent
|
||||
use GitHub pull request URLs:
|
||||
https://github.com/#{match_obj[1]}/#{match_obj[2]}/pull/#{match_obj[3]}.patch
|
||||
Rather than patch-diff:
|
||||
#{patch_url}
|
||||
EOS
|
||||
end
|
||||
|
||||
if regex_match_group(patch, %r{macports/trunk})
|
||||
problem "MacPorts patches should specify a revision instead of trunk:\n#{patch_url}"
|
||||
end
|
||||
|
||||
if regex_match_group(patch, %r{^http://trac\.macports\.org})
|
||||
problem "Patches from MacPorts Trac should be https://, not http:\n#{patch_url}"
|
||||
end
|
||||
|
||||
return unless regex_match_group(patch, %r{^http://bugs\.debian\.org})
|
||||
problem "Patches from Debian should be https://, not http:\n#{patch_url}"
|
||||
end
|
||||
end
|
||||
end
|
||||
end
|
||||
end
|
||||
@ -1,9 +1,9 @@
|
||||
require "rubocop"
|
||||
require "rubocop/rspec/support"
|
||||
require_relative "../../extend/string"
|
||||
require_relative "../../rubocops/legacy_patches_cop"
|
||||
require_relative "../../rubocops/patches_cop"
|
||||
|
||||
describe RuboCop::Cop::FormulaAudit::LegacyPatches do
|
||||
describe RuboCop::Cop::FormulaAudit::Patches do
|
||||
subject(:cop) { described_class.new }
|
||||
|
||||
context "When auditing legacy patches" do
|
||||
@ -47,6 +47,7 @@ describe RuboCop::Cop::FormulaAudit::LegacyPatches do
|
||||
"https://mirrors.ustc.edu.cn/macports/trunk/",
|
||||
"http://trac.macports.org/export/102865/trunk/dports/mail/uudeview/files/inews.c.patch",
|
||||
"http://bugs.debian.org/cgi-bin/bugreport.cgi?msg=5;filename=patch-libunac1.txt;att=1;bug=623340",
|
||||
"https://patch-diff.githubusercontent.com/raw/foo/foo-bar/pull/100.patch",
|
||||
]
|
||||
patch_urls.each do |patch_url|
|
||||
source = <<-EOS.undent
|
||||
@ -84,6 +85,15 @@ describe RuboCop::Cop::FormulaAudit::LegacyPatches do
|
||||
line: 5,
|
||||
column: 5,
|
||||
source: source }]
|
||||
elsif patch_url =~ %r{https?://patch-diff\.githubusercontent\.com/raw/(.+)/(.+)/pull/(.+)\.(?:diff|patch)}
|
||||
expected_offenses = [{ message: "use GitHub pull request URLs:\n"\
|
||||
" https://github.com/foo/foo-bar/pull/100.patch\n"\
|
||||
"Rather than patch-diff:\n"\
|
||||
" https://patch-diff.githubusercontent.com/raw/foo/foo-bar/pull/100.patch\n",
|
||||
severity: :convention,
|
||||
line: 5,
|
||||
column: 5,
|
||||
source: source }]
|
||||
end
|
||||
expected_offenses.zip([cop.offenses.last]).each do |expected, actual|
|
||||
expect_offense(expected, actual)
|
||||
@ -125,4 +135,67 @@ describe RuboCop::Cop::FormulaAudit::LegacyPatches do
|
||||
end
|
||||
end
|
||||
end
|
||||
|
||||
context "When auditing external patches" do
|
||||
it "Patch URLs" do
|
||||
patch_urls = [
|
||||
"https://raw.github.com/mogaal/sendemail",
|
||||
"https://mirrors.ustc.edu.cn/macports/trunk/",
|
||||
"http://trac.macports.org/export/102865/trunk/dports/mail/uudeview/files/inews.c.patch",
|
||||
"http://bugs.debian.org/cgi-bin/bugreport.cgi?msg=5;filename=patch-libunac1.txt;att=1;bug=623340",
|
||||
"https://patch-diff.githubusercontent.com/raw/foo/foo-bar/pull/100.patch",
|
||||
]
|
||||
patch_urls.each do |patch_url|
|
||||
source = <<-EOS.undent
|
||||
class Foo < Formula
|
||||
homepage "ftp://example.com/foo"
|
||||
url "http://example.com/foo-1.0.tgz"
|
||||
patch do
|
||||
url "#{patch_url}"
|
||||
sha256 "63376b8fdd6613a91976106d9376069274191860cd58f039b29ff16de1925621"
|
||||
end
|
||||
end
|
||||
EOS
|
||||
|
||||
inspect_source(cop, source)
|
||||
if patch_url =~ %r{/raw\.github\.com/}
|
||||
expected_offenses = [{ message: "GitHub/Gist patches should specify a revision:\n#{patch_url}",
|
||||
severity: :convention,
|
||||
line: 5,
|
||||
column: 16,
|
||||
source: source }]
|
||||
elsif patch_url =~ %r{macports/trunk}
|
||||
expected_offenses = [{ message: "MacPorts patches should specify a revision instead of trunk:\n#{patch_url}",
|
||||
severity: :convention,
|
||||
line: 5,
|
||||
column: 37,
|
||||
source: source }]
|
||||
elsif patch_url =~ %r{^http://trac\.macports\.org}
|
||||
expected_offenses = [{ message: "Patches from MacPorts Trac should be https://, not http:\n#{patch_url}",
|
||||
severity: :convention,
|
||||
line: 5,
|
||||
column: 9,
|
||||
source: source }]
|
||||
elsif patch_url =~ %r{^http://bugs\.debian\.org}
|
||||
expected_offenses = [{ message: "Patches from Debian should be https://, not http:\n#{patch_url}",
|
||||
severity: :convention,
|
||||
line: 5,
|
||||
column: 9,
|
||||
source: source }]
|
||||
elsif patch_url =~ %r{https?://patch-diff\.githubusercontent\.com/raw/(.+)/(.+)/pull/(.+)\.(?:diff|patch)}
|
||||
expected_offenses = [{ message: "use GitHub pull request URLs:\n"\
|
||||
" https://github.com/foo/foo-bar/pull/100.patch\n"\
|
||||
"Rather than patch-diff:\n"\
|
||||
" https://patch-diff.githubusercontent.com/raw/foo/foo-bar/pull/100.patch\n",
|
||||
severity: :convention,
|
||||
line: 5,
|
||||
column: 9,
|
||||
source: source }]
|
||||
end
|
||||
expected_offenses.zip([cop.offenses.last]).each do |expected, actual|
|
||||
expect_offense(expected, actual)
|
||||
end
|
||||
end
|
||||
end
|
||||
end
|
||||
end
|
||||
Loading…
x
Reference in New Issue
Block a user