Add new `requires_gem` API for declaring which gems a Cop needs #12186

amomchilov · 2023-09-03T22:02:03Z

Fixes #11920.

This PR is used by:

Before submitting the PR make sure the following are checked:

The PR relates to only one subject with a clear title and description in grammatically correct, complete sentences.
Wrote good commit messages.
Commit message starts with [Fix #issue-number] (if the related issue exists).
Feature branch is up-to-date with master (if not - rebase it).
Squashed related commits together.
Added tests.
Ran bundle exec rake default. It executes all tests and runs RuboCop on its own code.
Added an entry (file) to the changelog folder named {change_type}_{change_description}.md if the new code introduces user-observable changes. See changelog entry format for details.

amomchilov · 2023-09-03T22:03:24Z

lib/rubocop/cop/base.rb

+      # rubocop:disable Layout/ClassStructure
+      class << self
+        attr_reader :gem_requirements


I'm not quite sure what the correct layout is.

I also see class << self is pretty rare in this codebase, but singleton_class.attr_reader(:gem_requirements) would be weird.

amomchilov · 2023-09-03T22:12:07Z

lib/rubocop/cop/base.rb

+        #   using the same syntax as a Gemfile, e.g. ">= 1.2.3"
+        #
+        # @api public
+        def requires_gem(gem_name, version_requirement)


Should this be in a mixin, like rubocop-rails's TargetRailsVersion?

It'll keep the logic nicely separated and tested in its own file, but it means we couldn't use it from target_rails_version_from_bundler_lock_file which is already in the Base.

I'll have to think about this more, but I can live with the current proposal.

There's also the question whether we want this to be able to work with multiple gems, but I'm guessing the likelihood that a cop depends on multiple gems is pretty tiny, so let's keep it simple for now.

amomchilov · 2023-09-03T22:15:41Z

spec/rubocop/config_spec.rb

+
+    context 'and neither Gemfile.lock nor gems.locked exist' do
+      it 'returns nil' do
+        expect(configuration.gem_versions_in_target.nil?).to be(true)


I'm not used to RSpec, is this correct?

I originally had:

Suggested change

expect(configuration.gem_versions_in_target.nil?).to be(true)

expect(configuration.gem_versions_in_target).to be_nil

But rubocop auto-corrected to this. It feels a bit... funky

That's correct! expect(configuration.gem_versions_in_target).to be_nil

Hmm I'll try again, but the RuboCop settings on this project didn't like the use of predicate methods like be_nil

amomchilov · 2023-09-04T03:03:53Z

Hey @koic, I got all the tests to pass, but I'm having some issues Layout/ClassStructure. I think this PR is ready for an early review.

I'd appreciate any early thoughts or feedback you might have!

amomchilov · 2023-09-30T16:03:12Z

lib/rubocop/cop/team.rb

+        # In this case, the rails version was already checked by `#excluded_file?`
+        return true if defined?(RuboCop::Rails::TargetRailsVersion::USES_REQUIRES_GEM_API)


It looks like the versioning between rubocop and rubocop-rails gets a bit tricky here.

This new version of rubocop needs to support older versions of rubocop-rails.

Inversely, the new version of rubocop-rails needs to support older versions of rubocop.

I've tested both scenarios, and they both work well.

I added this USES_REQUIRES_GEM_API constant to detect new rubocop-rails versions, because it was more explicit/clear than checking against some specific version number.

In the future, TargetRailsVersion can just be a wrapper for requires_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 their support_target_rails_version? method.

amomchilov · 2023-09-30T16:04:43Z

Hey @bbatsov and @koic, I think this is ready for review. This was a bit of a tricky change for me, especially because it spans both rubocop and rubocop-rails.

I've tested it pretty thoroughly, and it all works correctly, but I would still be open to any feedback!

Cheers

bbatsov · 2023-10-11T10:43:48Z

Sorry for the slow turnaround here - I'm fine with the general direction, but I'll need some time before I can take a closer look at the code, as I've been very busy lately at work and wrapping up releases of some other OSS projects.

At a glance the one things that's currently missing is some user documentation for the proposed new feature.

bbatsov · 2023-10-11T10:48:21Z

It'd also be nice to benchmark the impact of using require_gem on several cops, as speed is something quite important for a tool like RuboCop.

amomchilov

@bbatsov I had to do a little bit of surgery, but I got this to work with the existing RuboCop::Lockfile wrapper for Bundler::LockfileParser.

I left some comments to explain my process, let me know what you think!

Still TODO:

Write user docs – I can do this
Benchmark this for perf regression – I'm not sure what the best way to do this is. Let me know if you have any suggestions

amomchilov · 2024-03-03T20:10:12Z

lib/rubocop/lockfile.rb

+    # Gems that the bundle directly depends on.
+    # @return [Array<Bundler::Dependency>, nil]


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 @api private, which get inherited by all of these methods, so there's no risk of people confusing them for public API (even if they're documented here).

amomchilov · 2024-03-03T20:11:31Z

lib/rubocop/lockfile.rb

@@ -5,6 +5,13 @@ module RuboCop
  # Does not actually resolve gems, just parses the lockfile.
  # @api private
  class Lockfile
+    # @param [String, Pathname, nil] lockfile_path
+    def initialize(lockfile_path: nil)
+      lockfile_path ||= defined?(Bundler) ? Bundler.default_lockfile : nil


I parameterized this class to let you specify your own lockfile_path, but kept the default behaviour the same as before.

This let's use it with the result from RuboCop::Config#bundler_lock_file_path.

amomchilov · 2024-03-03T20:12:41Z

lib/rubocop/lockfile.rb

+    # @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)


The pre-existing methods (#dependencies and #spec) return Gem::Dependency objects, which describe the version requirements, but not the resolved versions.

This method returns the actual resolved versions, locked by the lock file.

This is what we check gem requirements against.

bbatsov · 2024-03-03T21:08:45Z

Benchmark this for perf regression – I'm not sure what the best way to do this is. Let me know if you have any suggestions

You can simply test on some bigger Ruby repo (e.g. Rails).

@jonas054 @koic Please, take a look as well.

This class may be private, but its methods' behaviour is still pretty non-obvious, so let's document them. They're all marked `@api private`, so reader third parties should know not to use them.

amomchilov · 2024-03-05T02:14:14Z

Rebased to fix the unrelated style failure, which was fixed by #12737

jonas054 · 2024-03-06T19:40:32Z

Looks good to me. Some of it goes over my head a bit, but it looks like well-written and quite readable code. Good job on the documentation too.

amomchilov · 2024-03-06T21:04:17Z

@jonas054 Would you be okay to share which parts "go over your head a bit"? I'd love to clarify anything that might be confusing.

jonas054 · 2024-03-07T08:22:11Z

No, don't worry about that comment, @amomchilov. That was much more about my abilities to review code in parts of RuboCop that I'm only partly familiar with. I don't see any specific opportunities for improvement when it comes to clarity.

amomchilov · 2024-03-16T17:34:46Z

Ran a quick benchmark against Rails, which shows no discernable performance difference:

On the master branch:

$ time ../rubocop/rubocop/exe/rubocop --cache false
Inspecting 3265 files
...
3265 files inspected, no offenses detected
../rubocop/rubocop/exe/rubocop --cache false  61.37s user 0.77s system 95% cpu 1:04.84 total

On this branch:

$ time ../rubocop/rubocop/exe/rubocop --cache false
Inspecting 3265 files
...
3265 files inspected, no offenses detected
../rubocop/rubocop/exe/rubocop --cache false  61.69s user 0.74s system 97% cpu 1:04.02 total

@koic I think this is for a final review and merge. Could you please have a look?

bbatsov · 2024-03-29T14:50:31Z

If @koic doesn't share any feedback I'll merge this after the next bug-fix release.

amomchilov force-pushed the add-requires_gem-api branch from 5dcb450 to 7590743 Compare September 3, 2023 22:05

amomchilov mentioned this pull request Sep 3, 2023

Replace regex with Bundler::LockfileParser #12180

Merged

8 tasks

amomchilov commented Sep 3, 2023

View reviewed changes

amomchilov force-pushed the add-requires_gem-api branch 4 times, most recently from 2e28918 to c379d81 Compare September 4, 2023 03:02

amomchilov force-pushed the add-requires_gem-api branch 2 times, most recently from a79b732 to 4a09730 Compare September 4, 2023 15:42

amomchilov marked this pull request as ready for review September 26, 2023 14:49

amomchilov marked this pull request as draft September 26, 2023 14:49

amomchilov force-pushed the add-requires_gem-api branch from 4a09730 to 5cd56c3 Compare September 30, 2023 15:54

amomchilov marked this pull request as ready for review September 30, 2023 15:55

amomchilov commented Sep 30, 2023

View reviewed changes

amomchilov force-pushed the add-requires_gem-api branch 2 times, most recently from 626b18e to 9970f1b Compare March 3, 2024 20:14

amomchilov commented Mar 3, 2024

View reviewed changes

amomchilov force-pushed the add-requires_gem-api branch 2 times, most recently from de7dca7 to 9bfda44 Compare March 4, 2024 03:40

amomchilov added 5 commits March 4, 2024 21:11

Document RuboCop::Lockfile

d9f7d1d

This class may be private, but its methods' behaviour is still pretty non-obvious, so let's document them. They're all marked `@api private`, so reader third parties should know not to use them.

Parameterize lockfile path of RuboCop::Lockfile

91e43b1

Add method for retrieving locked gem versions

c805709

Add new Config#gem_versions_in_target API

b3c2d11

Add new requires_gem API

d11fa84

[Fix rubocop#11920] Skip files that don't satisfy gem version reqs

bf7faee

amomchilov force-pushed the add-requires_gem-api branch from 9bfda44 to bf7faee Compare March 5, 2024 02:12

bbatsov merged commit 2104ffd into rubocop:master Apr 2, 2024
33 checks passed

amomchilov mentioned this pull request Apr 6, 2024

Migrate TargetRailsVersion to the new requires_gem RuboCop API rubocop/rubocop-rails#1137

Merged

9 tasks

100terres mentioned this pull request Apr 8, 2024

bundle exec required for running rubocop 1.63.0 and rubocop-rails #12823

Closed

amomchilov mentioned this pull request Apr 9, 2024

[Fix #12823] Fix uninitialized constant error #12826

Merged

8 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add new `requires_gem` API for declaring which gems a Cop needs #12186

Add new `requires_gem` API for declaring which gems a Cop needs #12186

amomchilov commented Sep 3, 2023 •

edited

Loading

amomchilov Sep 3, 2023

amomchilov Sep 3, 2023

bbatsov Oct 11, 2023

amomchilov Sep 3, 2023

abzuar9658 Sep 6, 2023 •

edited

Loading

amomchilov Sep 6, 2023

amomchilov commented Sep 4, 2023 •

edited

Loading

amomchilov Sep 30, 2023

amomchilov commented Sep 30, 2023 •

edited

Loading

bbatsov commented Oct 11, 2023

bbatsov commented Oct 11, 2023

amomchilov left a comment •

edited

Loading

amomchilov Mar 3, 2024

amomchilov Mar 3, 2024

amomchilov Mar 3, 2024

bbatsov commented Mar 3, 2024

amomchilov commented Mar 5, 2024

jonas054 commented Mar 6, 2024

amomchilov commented Mar 6, 2024

jonas054 commented Mar 7, 2024

amomchilov commented Mar 16, 2024 •

edited

Loading

bbatsov commented Mar 29, 2024

	expect(configuration.gem_versions_in_target.nil?).to be(true)
	expect(configuration.gem_versions_in_target).to be_nil

		# In this case, the rails version was already checked by `#excluded_file?`
		return true if defined?(RuboCop::Rails::TargetRailsVersion::USES_REQUIRES_GEM_API)

		# Gems that the bundle directly depends on.
		# @return [Array<Bundler::Dependency>, nil]

Add new requires_gem API for declaring which gems a Cop needs #12186

Add new requires_gem API for declaring which gems a Cop needs #12186

Conversation

amomchilov commented Sep 3, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

abzuar9658 Sep 6, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amomchilov commented Sep 4, 2023 • edited Loading

Choose a reason for hiding this comment

amomchilov commented Sep 30, 2023 • edited Loading

bbatsov commented Oct 11, 2023

bbatsov commented Oct 11, 2023

amomchilov left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bbatsov commented Mar 3, 2024

amomchilov commented Mar 5, 2024

jonas054 commented Mar 6, 2024

amomchilov commented Mar 6, 2024

jonas054 commented Mar 7, 2024

amomchilov commented Mar 16, 2024 • edited Loading

bbatsov commented Mar 29, 2024

Add new `requires_gem` API for declaring which gems a Cop needs #12186

Add new `requires_gem` API for declaring which gems a Cop needs #12186

amomchilov commented Sep 3, 2023 •

edited

Loading

abzuar9658 Sep 6, 2023 •

edited

Loading

amomchilov commented Sep 4, 2023 •

edited

Loading

amomchilov commented Sep 30, 2023 •

edited

Loading

amomchilov left a comment •

edited

Loading

amomchilov commented Mar 16, 2024 •

edited

Loading