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 required_ruby_version issue when using Gem::Requirement #9037

Merged

Conversation

cetinajero
Copy link
Contributor

This fixes an issue when the required_ruby_version uses Gem::Requirement to define the Ruby version.

Gemspec:

...
s.required_ruby_version = Gem::Requirement.new('< 3.0.0')
...

Backtrace using v1.3.0:

undefined method `match' for nil:NilClass
296/home/travis/build/grupopv/jekyll-tasks/vendor/bundle/ruby/2.5.0/gems/rubocop-1.3.0/lib/rubocop/target_ruby.rb:165:in `version_from_str'
297/home/travis/build/grupopv/jekyll-tasks/vendor/bundle/ruby/2.5.0/gems/rubocop-1.3.0/lib/rubocop/target_ruby.rb:144:in `find_version'
298/home/travis/build/grupopv/jekyll-tasks/vendor/bundle/ruby/2.5.0/gems/rubocop-1.3.0/lib/rubocop/target_ruby.rb:22:in `initialize'

Related to #8761


Before submitting the PR make sure the following are checked:

  • 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.
  • 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.
  • The PR relates to only one subject with a clear title and description in grammatically correct, complete sentences.
  • Run bundle exec rake default. It executes all tests and RuboCop for itself, and generates the documentation.

This fixes an issue when the `required_ruby_version` uses `Gem::Requirement` to define the Ruby version.

e.g.

```
s.required_ruby_version = Gem::Requirement.new('< 3.0.0')
```

Related to rubocop#8761
@@ -141,9 +141,17 @@ def find_version
return versions.compact.min
end

return gem_requirement_version(version) if version.send_type?
Copy link
Collaborator

Choose a reason for hiding this comment

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

Probably we should do a more precise node match here, as there's a slight chance some other code might be used here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks @bbatsov!

There is now a more precise matching pattern.

Copy link
Contributor

Choose a reason for hiding this comment

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

Looks good! Maybe you can merge it with if version.array_type?, also capturing the versions, and you can reuse the same map, compact, min. Maybe not though, just a thought

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'll look into it @marcandre!

Copy link
Contributor

Choose a reason for hiding this comment

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

{ (array $str+) ... } can help, and there's https://nodepattern.herokuapp.com/ to help test things.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So now the map, compact and min are reused and I definitely think it looks more DRY.

I managed to merge it by creating a version_from_array method but didn't do any new NodePattern.

version_from_str(version.str_content)
end

def gem_requirement_version(version)
Copy link
Collaborator

Choose a reason for hiding this comment

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

As this is still extracted from the gemspec it should probably be part of that existing method.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

New arrangement based on #9037 (comment), let me know your thoughts @bbatsov .

@cetinajero cetinajero force-pushed the required_ruby_version_as_requirement branch from ca1ae44 to 03bc071 Compare November 13, 2020 18:46
@cetinajero cetinajero force-pushed the required_ruby_version_as_requirement branch from 03bc071 to b602d9c Compare November 13, 2020 18:51
@bbatsov bbatsov merged commit 9e83b37 into rubocop:master Nov 13, 2020
@bbatsov
Copy link
Collaborator

bbatsov commented Nov 13, 2020

Looks good! Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants