[Fix #9803] Fix Bundler/GemVersion cop not respecting git tags
#9807
Conversation
|
Can you add a test that uses Also I wonder if |
Added specs with
I don't think so. Tags are usually fixed. Branch reference keeps changing, so it is not really fixing a version. |
| (send nil? :gem <(str #version_specification?) ...>) | ||
| (send nil? :gem | ||
| <{(str #version_specification?) | ||
| (hash <(pair (sym :tag) (str _)) (pair (sym {:git :github}) (str _)) ...>)} ...>) |
There was a problem hiding this comment.
Is matching git or github required? Would (sym :tag) be enough to indicate tag presence? That way the cop doesn't need to be concerned about the gem source.
If it is needed, it looks like bitbucket is also a valid option to provide.
There was a problem hiding this comment.
I did not want this to report false positives - hence the extra check.
Thanks I was not aware of the bitbucket option. Looks like there is gist as well. I'll add them both.
There was a problem hiding this comment.
Added bitbucket only, not sure if gist allows versions. I mean it has revisions, but the gist ID remains same.
There was a problem hiding this comment.
Users can also add their own arbitrary git sources using git_source so we should probably handle that as well.
There was a problem hiding this comment.
@dvandersluis I think this might be tricky given that we can define custom sources:
git_source(:stash) { |repo_name| "https://stash.corp.acme.pl/#{repo_name}.git" }
...
gem 'rails', :stash => 'forks/rails'There was a problem hiding this comment.
I agree it's tricky, but if we don't handle it someway it leads to false reporting (which is maybe another argument for just looking for tag/ref/etc. and not caring about the other keys).
| gem 'rubocop', github: 'rubocop/rubocop', tag: 'v1' | ||
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Gem version specification is forbidden. | ||
| gem 'rubocop', git: 'https://github.com/rubocop/rubocop', tag: 'v1' | ||
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Gem version specification is forbidden. |
There was a problem hiding this comment.
These look strange. If :tag or :ref is specified with :git or :github, it's likely that a meaningful version of commit is being used. So, shouldn't these cases always be accepted?
There was a problem hiding this comment.
This is for 'EnforcedStyle' => 'forbidden' which forbids fixing a gem version (or in this case an equivalent git tag).
There was a problem hiding this comment.
Practically, if :tag or :ref with :git or :github is used, the pinned value should be respected. AFAIK, even 'EnforcedStyle' => 'forbidden' should not be forbidden, as it is used for intentional forks and the like.
| @@ -24,6 +26,9 @@ | |||
| gem 'rubocop', '~> 1' | |||
| gem 'rubocop', '~> 1.12', require: false | |||
| gem 'rubocop', '>= 1.5.0', '< 1.10.0', git: 'https://github.com/rubocop/rubocop' | |||
| gem 'rubocop', github: 'rubocop/rubocop', tag: 'v1' | |||
| gem 'rubocop', git: 'https://github.com/rubocop/rubocop', tag: 'v1' | |||
| gem 'rubocop', bitbucket: 'https://bitbucket.com/foo/bar', tag: 'v1' | |||
There was a problem hiding this comment.
:ref can be accepted as well as :tag.
tejasbubane
force-pushed
the
fix-9803
branch
2 times, most recently
from
4f52656
to
de955d1
Compare
|
Two bits of feedback from me:
|
| @@ -45,6 +45,21 @@ class GemVersion < Base | |||
| (send nil? :gem <(str #version_specification?) ...>) | |||
| PATTERN | |||
|
|
|||
| # @!method with_git_ref?(node) | |||
| def_node_matcher :with_git_ref?, <<~PATTERN | |||
There was a problem hiding this comment.
I'd use a more neutral name here - e.g. commit_ref?.
| PATTERN | ||
|
|
||
| # @!method git?(node) | ||
| def_node_matcher :git?, <<~PATTERN |
There was a problem hiding this comment.
Do we really need to change for the VCS? I doubt someone would use :tag or :ref without them and this will probably result in some mistake. Haven't tried it, though.
|
@tejasbubane ping :-) |
|
Superseded by #9885. |
Closes #9803
Before submitting the PR make sure the following are checked:
[Fix #issue-number](if the related issue exists).master(if not - rebase it).bundle exec rake default. It executes all tests and runs RuboCop on its own code.{change_type}_{change_description}.mdif the new code introduces user-observable changes. See changelog entry format for details.