Required ruby version fixes #9515

HeroProtagonist · 2021-02-14T04:14:03Z

The class to match required_ruby_version from .gemspec files could return invalid ruby versions.

This PR makes it so that it will only return a value from the KNOWN_RUBIES array. Specifically the minimal known ruby will be returned.
The version parsing is also greatly improved by leveraging existing classes from the Gem library.

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.

Add cases that include - numeric version - approximate version - not equal version - array of many version types

…equired_ruby_version` - Create `Gem::Requirement` from required_ruby_version - Return first `KNOWN_RUBY` that satisfies the requirement when one exists - Support more types of version entries

marcandre

Much better!
I think it can be improved, simplified, and avoid parsing the requirement ourselves

marcandre · 2021-02-14T15:10:52Z

lib/rubocop/target_ruby.rb

+
+      # Helper to check if the requirement is satisfied by a given known ruby version
+      # @api private
+      class Matcher


I think it would be best to do without this class and use the Gem::Requirement as a matcher. Note that matchers typically better constructed without the value to match; that value is passed to the matching method (like satisfied_by?).

marcandre · 2021-02-14T15:13:39Z

lib/rubocop/target_ruby.rb

-      def version_from_str(str)
-        str.match(/^(?:>=|<=)?\s*(?<version>\d+(?:\.\d+)*)/) do |md|
-          md[:version].to_f
+      def remove_patch_from_version(str)


I see that you went through the trouble of parsing the requirement nevertheless... How about instead mapping our versions like 2.7 => '2.7.999'? This would have the desired effect, except in the case of exact Ruby version requirements (which are a mistake), simplify the code and avoid redoing the parsing that we shouldn't be doing.

marcandre · 2021-02-14T15:15:28Z

spec/rubocop/target_ruby_spec.rb

                  s.licenses = ['MIT']
                end
              HEREDOC

            create_file(gemspec_file_path, content)
            expect(target_ruby.version).to eq default_version
          end
+
+          it 'sets target_ruby from inclusive range' do
+            content =


No obligation, but consider using let or before_each to avoid duplicating all these lines so many times.

marcandre · 2021-02-14T15:16:47Z

spec/rubocop/target_ruby_spec.rb

            content =
              <<-HEREDOC
                Gem::Specification.new do |s|
                  s.name = 'test'
-                  s.required_ruby_version = '>= 3.0.0'
+                  s.required_ruby_version = '2.7.4'


I wouldn't even test this. There's is no reason I can think of to specify such a requirement. We could even consider raising an offense in one of our cops for this, but I doubt it happens in practice.

marcandre · 2021-02-14T15:18:56Z

spec/rubocop/target_ruby_spec.rb

            content =
              <<-HEREDOC
                Gem::Specification.new do |s|
                  s.name = 'test'
-                  s.required_ruby_version = '< 3.0.0'
+                  s.required_ruby_version = '2.3.0'


Same here, testing for < 2.3.0 is fine, but not = 2.3.0

HeroProtagonist · 2021-02-16T04:01:39Z

@marcandre Thanks for the review. Updated the PR and I think its ready now 🤞

marcandre

Yes, it is almost ready 😅

marcandre · 2021-02-16T05:27:53Z

lib/rubocop/target_ruby.rb

+        gem_version = ->(ruby_version) { Gem::Version.new(ruby_version) }
+
+        match = KNOWN_RUBIES.each.lazy.map(&gem_version).detect { |v| requirement.satisfied_by?(v) }


Can't these lines be replaced by:

match = KNOWN_RUBIES.detect { |v| requirement.satisfied_by?(Gem::Version.new(v) }

I'd actually use Gem::Version.new("#{v}.99") so that it works ok for >= '2.6.1'

In general, storing a lambda in a local variable is an anti-pattern. Also, it's typically best to use lazy only when actually needed.

Only one question. The .99 would cause a not equal check like the ones I have here and here to fail. Maybe that is ok? Not sure how common it is to be specifying not equal versions

Seems to me it is an error to use !=. In any case, if someone did specify != 2.4.0, that doesn't exclude 2.4.1 etc which are in the 2.4 series, so it seems like an ok target.

The main scenario we have to get right is >, >= and potentially ~> even though that seems a bit strange.

Ok 1 more thing if we had '> 2.4.1' then it would match to 2.4 due to the .99, that seems wrong?

I think that's the best behavior. Imagine that there is a bug in 2.4.1 that is fixed in Ruby 2.4.2+; the gem author wants to make sure we don't try to run the gem in 2.4.1 or below as the bug can be triggered by code in the gem. The gem still uses the "2.4.x" features and syntax, so target of 2.4 is best.

Ok pushed the updates 😃

marcandre · 2021-02-16T05:28:17Z

spec/rubocop/target_ruby_spec.rb

            content =
              <<-HEREDOC
                Gem::Specification.new do |s|
                  s.name = 'test'
-                  s.required_ruby_version = '2.7.4'
+                  s.required_ruby_version = '>= 2.6.0'


It should also work for >= 2.6.1

marcandre

Good stuff! 💪
Thanks 😄

HeroProtagonist force-pushed the required-ruby-version-fixes branch 2 times, most recently from 38e6f2c to bb076a8 Compare February 14, 2021 04:32

bbatsov approved these changes Feb 14, 2021

View reviewed changes

bbatsov requested a review from marcandre February 14, 2021 09:54

HeroProtagonist added 3 commits February 14, 2021 08:02

[Fix rubocop#9482] Update tests for gemspec file

d2b2deb

Add cases that include - numeric version - approximate version - not equal version - array of many version types

[Fix rubocop#9482] Return minimal known ruby version from gemspecs `r…

e07f469

…equired_ruby_version` - Create `Gem::Requirement` from required_ruby_version - Return first `KNOWN_RUBY` that satisfies the requirement when one exists - Support more types of version entries

[Fix rubocop#9482] Generate changelog entry

4e15000

HeroProtagonist force-pushed the required-ruby-version-fixes branch from bb076a8 to 4e15000 Compare February 14, 2021 13:02

marcandre requested changes Feb 14, 2021

View reviewed changes

bbatsov assigned marcandre Feb 15, 2021

marcandre requested changes Feb 16, 2021

View reviewed changes

ashkulz mentioned this pull request Feb 18, 2021

Change to require rubocop 1.0.0 or above prontolabs/pronto-rubocop#60

Merged

[Fix rubocop#9482] Address PR review

b3093ba

HeroProtagonist force-pushed the required-ruby-version-fixes branch from d80cfc5 to b3093ba Compare February 20, 2021 01:17

marcandre approved these changes Feb 20, 2021

View reviewed changes

marcandre merged commit 3ade276 into rubocop:master Feb 20, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Required ruby version fixes #9515

Required ruby version fixes #9515

HeroProtagonist commented Feb 14, 2021

marcandre left a comment

marcandre Feb 14, 2021

marcandre Feb 14, 2021

marcandre Feb 14, 2021

marcandre Feb 14, 2021

marcandre Feb 14, 2021

HeroProtagonist commented Feb 16, 2021

marcandre left a comment

marcandre Feb 16, 2021

HeroProtagonist Feb 17, 2021

marcandre Feb 17, 2021 •

edited

HeroProtagonist Feb 18, 2021

marcandre Feb 18, 2021

HeroProtagonist Feb 20, 2021 •

edited

marcandre Feb 16, 2021

marcandre left a comment

		gem_version = ->(ruby_version) { Gem::Version.new(ruby_version) }

		match = KNOWN_RUBIES.each.lazy.map(&gem_version).detect { \|v\| requirement.satisfied_by?(v) }

Required ruby version fixes #9515

Required ruby version fixes #9515

Conversation

HeroProtagonist commented Feb 14, 2021

marcandre left a comment

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

HeroProtagonist commented Feb 16, 2021

marcandre left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marcandre Feb 17, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

HeroProtagonist Feb 20, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marcandre left a comment

Choose a reason for hiding this comment

marcandre Feb 17, 2021 •

edited

HeroProtagonist Feb 20, 2021 •

edited