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: Migrate scheme checks for cvs
, bzr
, hg
, fossil
and svn+http
to Rubocop
+ add tests
#7619
audit: Migrate scheme checks for cvs
, bzr
, hg
, fossil
and svn+http
to Rubocop
+ add tests
#7619
Conversation
@@ -66,6 +66,11 @@ def audit_formula(_node, _class_node, _parent_class_node, body_node) | |||
problem "#{url} should be `https://www.apache.org/dyn/closer.lua?path=#{match[1]}`" | |||
end | |||
|
|||
version_control_pattern = %r{^(cvs|bzr|hg|fossil)://} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure about where to best put it in this file. Any input appreciated.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems fine to me! Alternative would be a VERSION_CONTROL_PATTERN = %r{^(cvs|bzr|hg|fossil)://}.freeze
at the class level.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see, thanks for pointing out.
Btw, I also tried this, to have all the vcs stuff combined:
version_control_patterns = Regexp.union([%r{^(cvs|bzr|hg|fossil)://},
%r{^(svn)\+http://}])
audit_urls(urls, version_control_patterns) do |match, _|
problem "Use of the #{match[0]} scheme is deprecated, pass `:using => :#{match[1]}` instead"
end
But then the problem is that the matching svn
is not contained in match[1]
, but rather match[2]
. I'm not an expert with Regexp.union
. Maybe you have an idea to optimize this code :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeh, I've tried and can't find a way to match them consistently unfortunately. Two regex seems fine 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I used https://rubular.com for testing in case that's useful to you.
cvs
, bzr
, hg
and fossil
to Rubocop
cvs
, bzr
, hg
and fossil
to Rubocop
+ add tests
19568f1
to
bb9665c
Compare
cvs
, bzr
, hg
and fossil
to Rubocop
+ add testscvs
, bzr
, hg
, fossil
and svn+http
to Rubocop
+ add tests
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work again @mathaeus!
@@ -66,6 +66,11 @@ def audit_formula(_node, _class_node, _parent_class_node, body_node) | |||
problem "#{url} should be `https://www.apache.org/dyn/closer.lua?path=#{match[1]}`" | |||
end | |||
|
|||
version_control_pattern = %r{^(cvs|bzr|hg|fossil)://} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems fine to me! Alternative would be a VERSION_CONTROL_PATTERN = %r{^(cvs|bzr|hg|fossil)://}.freeze
at the class level.
Migrate scheme checks for
cvs
,bzr
,hg
,fossil
andsvn+http
toRubocop
.Motivated by #7392
brew style
with your changes locally?brew tests
with your changes locally?