Skip to content
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

[Fix #7669] Add Bundler/GemVersion cop #9727

Merged
merged 8 commits into from May 5, 2021
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions changelog/new_gem_version_declaration.md
@@ -0,0 +1 @@
* [#7669](https://github.com/rubocop/rubocop/issues/7669): New cop `Bundler/GemVersionDeclaration` requires or prohibits specifying gem versions. ([@timlkelly][])
12 changes: 12 additions & 0 deletions config/default.yml
Expand Up @@ -174,6 +174,18 @@ Bundler/GemComment:
IgnoredGems: []
OnlyFor: []

Bundler/GemVersionDeclaration:
Description: 'Requires or prohibits gem version declarations.'
Enabled: false
VersionAdded: '<<next>>'
EnforcedStyle: 'required'
SupportedStyles:
- 'required'
- 'prohibited'
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we've typically used "forbidden" in the codebase, but you'll have to check this.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Forbidden definitely seems to be more common than prohibited, good to know!

Include:
- '**/Gemfile'
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I saw that other Bundler cops also include *.gemfile and gems.rb. I've personally never worked with this format of gem file, so I was unsure what exactly to test.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The format is exactly the same, it's just a different filename.

IgnoredGems: []
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lately I've favoured more names like "AllowedGems" instead of "IgnoredGems". Sadly, our use of both is a bit inconsistent.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 for AllowedGems.


Bundler/InsecureProtocolSource:
Description: >-
The source `:gemcutter`, `:rubygems` and `:rubyforge` are deprecated
Expand Down
1 change: 1 addition & 0 deletions lib/rubocop.rb
Expand Up @@ -148,6 +148,7 @@

require_relative 'rubocop/cop/bundler/duplicated_gem'
require_relative 'rubocop/cop/bundler/gem_comment'
require_relative 'rubocop/cop/bundler/gem_version_declaration'
require_relative 'rubocop/cop/bundler/insecure_protocol_source'
require_relative 'rubocop/cop/bundler/ordered_gems'

Expand Down
101 changes: 101 additions & 0 deletions lib/rubocop/cop/bundler/gem_version_declaration.rb
@@ -0,0 +1,101 @@
# frozen_string_literal: true

module RuboCop
module Cop
module Bundler
# Enforce that Gem version declarations are either required
# or prohibited.
#
# @example EnforcedStyle: required (default)
# # bad
# gem 'rubocop'
#
# # good
# gem 'rubocop', '~> 1.12'
#
# # good
# gem 'rubocop', '>= 1.10.0'
#
# # good
# gem 'rubocop', '>= 1.5.0', '< 1.10.0'
#
# @example EnforcedStyle: prohibited
# # good
# gem 'rubocop'
#
# # bad
# gem 'rubocop', '~> 1.12'
#
# # bad
# gem 'rubocop', '>= 1.10.0'
#
# # bad
# gem 'rubocop', '>= 1.5.0', '< 1.10.0'
#
class GemVersionDeclaration < Base
include ConfigurableEnforcedStyle

REQUIRED_MSG = 'Gem version declaration is required.'
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

declaration -> specification.

PROHIBITED_MSG = 'Gem version declaration is prohibited.'
VERSION_DECLARATION_REGEX = /^[~<>=]*\s?[0-9.]+/.freeze
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
VERSION_DECLARATION_REGEX = /^[~<>=]*\s?[0-9.]+/.freeze
VERSION_DECLARATION_REGEX = /^\s*[~<>=]*\s*[0-9.]+/.freeze

Multiple spaces are valid in bundler before and after the operator. We also could be more specific about the format here, but it probably doesn't really matter.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've added the suggested change.

We also could be more specific about the format here

Do you mind detailing what you mean by this? Or is it the change you provided?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What I mean is the Regexp will match ...... or >>>>> 7 or other random strings that aren’t version specifications, but again it probably doesn't really matter.

It could be more strict with something like ^\s*(?:~>|[<>]?=?)?\s*[0-9]+(\.[0-9]+)*


# @!method gem_declaration?(node)
def_node_matcher :gem_declaration?, '(send nil? :gem str ...)'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This method is in Bundler::GemComment as well, it might be useful to extract them to a shared mixin.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup, definitely. I did copy this from Bundler::GemComment to get it working originally. But I completely agree that a mixin will DRY this up.


# @!method includes_version_declaration?(node)
def_node_matcher :includes_version_declaration?, <<~PATTERN
(send nil? :gem <(str #version_declaration?) ...>)
PATTERN

def on_send(node)
return unless gem_declaration?(node)
return if ignored_gem?(node)

if offense?(node)
add_offense(node)
opposite_style_detected
else
correct_style_detected
end
end

private

def ignored_gem?(node)
ignored_gems.include?(node.first_argument.value)
end

def ignored_gems
Array(cop_config['IgnoredGems'])
end

def message(range)
gem_declaration = range.source

if required_style?
format(REQUIRED_MSG, gem_declaration: gem_declaration)
elsif prohibited_style?
format(PROHIBITED_MSG, gem_declaration: gem_declaration)
end
end

def offense?(node)
(required_style? && !includes_version_declaration?(node)) ||
(prohibited_style? && includes_version_declaration?(node))
end

def prohibited_style?
style == :prohibited
end

def required_style?
style == :required
end

def version_declaration?(expression)
expression.match?(VERSION_DECLARATION_REGEX)
end
end
end
end
end
71 changes: 71 additions & 0 deletions spec/rubocop/cop/bundler/gem_version_declaration_spec.rb
@@ -0,0 +1,71 @@
# frozen_string_literal: true

RSpec.describe RuboCop::Cop::Bundler::GemVersionDeclaration, :config do
context 'when EnforcedStyle is set to required (default)' do
let(:cop_config) do
{
'EnforcedStyle' => 'required',
'IgnoredGems' => ['rspec']
}
end

it 'flags gems without a version declaration' do
expect_offense(<<~RUBY)
gem 'rubocop'
^^^^^^^^^^^^^ Gem version declaration is required.
gem 'rubocop', require: false
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Gem version declaration is required.
RUBY
end

it 'ignores gems with a declared version' do
expect_no_offenses(<<~RUBY)
gem 'rubocop', '>=1.10.0'
gem 'rubocop', '~> 1'
gem 'rubocop', '~> 1.12', require: false
gem 'rubocop', '>= 1.5.0', '< 1.10.0', git: 'https://github.com/rubocop/rubocop'
RUBY
end

it 'ignores gems included in IgnoredGems metadata' do
expect_no_offenses(<<~RUBY)
gem 'rspec'
RUBY
end
end

context 'when EnforcedStyle is set to prohibited' do
let(:cop_config) do
{
'EnforcedStyle' => 'prohibited',
'IgnoredGems' => ['rspec']
}
end

it 'flags gems with a version declaration' do
expect_offense(<<~RUBY)
gem 'rubocop', '~> 1'
^^^^^^^^^^^^^^^^^^^^^ Gem version declaration is prohibited.
gem 'rubocop', '>=1.10.0'
^^^^^^^^^^^^^^^^^^^^^^^^^ Gem version declaration is prohibited.
gem 'rubocop', '~> 1.12', require: false
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Gem version declaration is prohibited.
gem 'rubocop', '>= 1.5.0', '< 1.10.0', git: 'https://github.com/rubocop/rubocop'
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Gem version declaration is prohibited.
RUBY
end

it 'ignores gems without a declared version' do
expect_no_offenses(<<~RUBY)
gem 'rubocop'
gem 'rubocop', require: false
RUBY
end

it 'ignores gems included in IgnoredGems metadata' do
expect_no_offenses(<<~RUBY)
gem 'rspec', '~> 3.10'
RUBY
end
end
end