-
-
Notifications
You must be signed in to change notification settings - Fork 3k
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
Add new requires_gem
API for declaring which gems a Cop needs
#12186
Changes from all commits
d9f7d1d
91e43b1
c805709
b3c2d11
d11fa84
bf7faee
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
* [#12186](https://github.com/rubocop/rubocop/pull/12186): Add new `requires_gem` API for declaring which gems a Cop needs. ([@amomchilov][]) |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -5,14 +5,23 @@ module RuboCop | |
# Does not actually resolve gems, just parses the lockfile. | ||
# @api private | ||
class Lockfile | ||
# Gems that the bundle depends on | ||
# @param [String, Pathname, nil] lockfile_path | ||
def initialize(lockfile_path = nil) | ||
lockfile_path ||= defined?(Bundler) ? Bundler.default_lockfile : nil | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I parameterized this class to let you specify your own This let's use it with the result from |
||
|
||
@lockfile_path = lockfile_path | ||
end | ||
|
||
# Gems that the bundle directly depends on. | ||
# @return [Array<Bundler::Dependency>, nil] | ||
Comment on lines
+15
to
+16
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. These methods' semantics aren't obvious (in particular, whether they include indirect dependencies or not, and what type of results they vend), so I documented them all. The whole class is marked |
||
def dependencies | ||
return [] unless parser | ||
|
||
parser.dependencies.values | ||
end | ||
|
||
# All activated gems, including transitive dependencies | ||
# All activated gems, including transitive dependencies. | ||
# @return [Array<Bundler::Dependency>, nil] | ||
def gems | ||
return [] unless parser | ||
|
||
|
@@ -21,17 +30,38 @@ def gems | |
parser.dependencies.values.concat(parser.specs.flat_map(&:dependencies)) | ||
end | ||
|
||
# Returns the locked versions of gems from this lockfile. | ||
# @param [Boolean] include_transitive_dependencies: When false, only direct dependencies | ||
# are returned, i.e. those listed explicitly in the `Gemfile`. | ||
# @returns [Hash{String => Gem::Version}] The locked gem versions, keyed by the gems' names. | ||
def gem_versions(include_transitive_dependencies: true) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The pre-existing methods ( This method returns the actual resolved versions, locked by the lock file. This is what we check gem requirements against. |
||
return {} unless parser | ||
|
||
all_gem_versions = parser.specs.to_h { |spec| [spec.name, spec.version] } | ||
|
||
if include_transitive_dependencies | ||
all_gem_versions | ||
else | ||
direct_dep_names = parser.dependencies.keys | ||
all_gem_versions.slice(*direct_dep_names) | ||
end | ||
end | ||
|
||
# Whether this lockfile includes the named gem, directly or indirectly. | ||
# @param [String] name | ||
# @return [Boolean] | ||
def includes_gem?(name) | ||
gems.any? { |gem| gem.name == name } | ||
end | ||
|
||
private | ||
|
||
# @return [Bundler::LockfileParser, nil] | ||
def parser | ||
return unless defined?(Bundler) && Bundler.default_lockfile | ||
return @parser if defined?(@parser) | ||
return unless @lockfile_path | ||
|
||
lockfile = Bundler.read_file(Bundler.default_lockfile) | ||
lockfile = Bundler.read_file(@lockfile_path) | ||
@parser = lockfile ? Bundler::LockfileParser.new(lockfile) : nil | ||
rescue Bundler::BundlerError | ||
nil | ||
|
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -825,6 +825,56 @@ def cop_enabled(cop_class) | |||||
end | ||||||
end | ||||||
|
||||||
describe '#gem_versions_in_target', :isolated_environment do | ||||||
['Gemfile.lock', 'gems.locked'].each do |file_name| | ||||||
let(:base_path) { configuration.base_dir_for_path_parameters } | ||||||
let(:lockfile_path) { File.join(base_path, file_name) } | ||||||
|
||||||
context "and #{file_name} exists" do | ||||||
it 'returns the locked gem versions' do | ||||||
content = | ||||||
<<~LOCKFILE | ||||||
GEM | ||||||
remote: https://rubygems.org/ | ||||||
specs: | ||||||
a (1.1.1) | ||||||
b (2.2.2) | ||||||
c (3.3.3) | ||||||
d (4.4.4) | ||||||
a (= 1.1.1) | ||||||
b (>= 1.1.1, < 3.3.3) | ||||||
c (~> 3.3) | ||||||
|
||||||
PLATFORMS | ||||||
ruby | ||||||
|
||||||
DEPENDENCIES | ||||||
rails (= 4.1.0) | ||||||
|
||||||
BUNDLED WITH | ||||||
2.4.19 | ||||||
LOCKFILE | ||||||
|
||||||
expected = { | ||||||
'a' => Gem::Version.new('1.1.1'), | ||||||
'b' => Gem::Version.new('2.2.2'), | ||||||
'c' => Gem::Version.new('3.3.3'), | ||||||
'd' => Gem::Version.new('4.4.4') | ||||||
} | ||||||
|
||||||
create_file(lockfile_path, content) | ||||||
expect(configuration.gem_versions_in_target).to eq expected | ||||||
end | ||||||
end | ||||||
end | ||||||
|
||||||
context 'and neither Gemfile.lock nor gems.locked exist' do | ||||||
it 'returns nil' do | ||||||
expect(configuration.gem_versions_in_target.nil?).to be(true) | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not used to RSpec, is this correct? I originally had:
Suggested change
But rubocop auto-corrected to this. It feels a bit... funky There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That's correct! There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hmm I'll try again, but the RuboCop settings on this project didn't like the use of predicate methods like |
||||||
end | ||||||
end | ||||||
end | ||||||
|
||||||
describe '#for_department', :restore_registry do | ||||||
let(:hash) do | ||||||
{ | ||||||
|
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.
It looks like the versioning between
rubocop
andrubocop-rails
gets a bit tricky here.rubocop
needs to support older versions ofrubocop-rails
.rubocop-rails
needs to support older versions ofrubocop
.I've tested both scenarios, and they both work well.
I added this
USES_REQUIRES_GEM_API
constant to detect newrubocop-rails
versions, because it was more explicit/clear than checking against some specific version number.In the future,
TargetRailsVersion
can just be a wrapper forrequires_gem
. When that happens, any cops that don't support the target's rails version would have already be filtered out by#excluded_file?
called by#roundup_relevant_cops
above.For older versions of
rubocop-rails
, that won't be correct, and we'll still need to call theirsupport_target_rails_version?
method.