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

Add new Rails/WhereRange cop #1272

Merged
merged 1 commit into from
May 2, 2024
Merged

Conversation

fatkodima
Copy link
Contributor

Closes #689.

It supports where and where.not, where with a a regular and an array syntax.

# bad
User.where('age >= ?', 18)
User.where.not('age >= ?', 18)
User.where('age < ?', 18)
User.where('age >= ? AND age < ?', 18, 21)
User.where('age >= :start', start: 18)
User.where('users.age >= ?', 18)

# good
User.where(age: 18..)
User.where.not(age: 18..)
User.where(age: ...18)
User.where(age: 18...21)
User.where(users: { age: 18.. })

# good
# There are no beginless ranges in ruby.
User.where('age > ?', 18)

@pirj
Copy link
Member

pirj commented Apr 26, 2024

The original ticket mentions a chain of where.

Also, for cases with an open-ended range the difference is not that critical. Can there be an option to strictly prefer ranges, or prefer ranges to double ‘where’ and ‘AND’?

@pirj
Copy link
Member

pirj commented Apr 26, 2024

And thanks for handling this! ❤️

@fatkodima
Copy link
Contributor Author

The original ticket mentions a chain of where.

That will make this cop much more complex. I would like to avoid that. Most of the offences, I believe, are defined via a single where call.

@pirj
Copy link
Member

pirj commented Apr 27, 2024

At a glance, we need to consider what ruby (2.6, 2.7+ or eaerlier) and Rails versions are our targets. Depending on this, suggestions may not apply. See this styleguide note and this comment.

there is nothing that would replace a set of conditions where one is inclusive, and the other is exclusive:
'foo >= ? AND foo < ?'

The cop does consider this, right?

@pirj
Copy link
Member

pirj commented Apr 27, 2024

will make this cop much more complex

I understand that, and I see that even now the cop is far from trivial.
My concern is that the cop would turn ‘where(“a > ?”, …).where(“a < ?”, …)’ into a chain of two ‘where’s, each with an open range. And combining ranges manually when there’s no cop to help is prone to this pitfall with a combination of inclusive / exclusive ranges.

RUBY

expect_correction(<<~RUBY)
Model.where(column: value..)
Copy link
Member

Choose a reason for hiding this comment

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

Endless ranges (start..) are supported since Ruby 2.6, while beginless ranges (..end) are supported since Ruby 2.7. Each should be considered accordingly.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ruby 2.7 is EOL for a long time. Not to mention 2.6.
Instead, wdyt about dropping the support for it altogether from the gem?

Copy link
Member

Choose a reason for hiding this comment

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

The runtime Ruby version (2.7+) and parsing Ruby version (2.0+) are different. This is the parsing Ruby version.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do you mean by the original comment, that I should update the test cases only or the code of the cop too?

Copy link
Member

Choose a reason for hiding this comment

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

RSpec supports Ruby 1.8.7
RuboCop “Targets Ruby 2.0+ code analysis”.
RuboCop Rails:

Rails cops support the following versions:
Rails 4.2+

Please don’t get me wrong, I always advocate to drop support for EOL versions, even in minor/patch versions. But while those versions are supported, we need to be compatible.

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 added minimum_target_ruby_version 2.7. Do you mean to lower it to 2.6 and handle both cases of endless ranges support differently?

Copy link
Member

Choose a reason for hiding this comment

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

I think it’s just perfect. My apologies for my ambiguous language, I meant not to break it for earlier ruby/rails versions.

Copy link
Member

Choose a reason for hiding this comment

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

Do you mean to lower it to 2.6 and handle both cases of endless ranges support differently?

Yes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated. Please, take a look.

Comment on lines 67 to 77
it 'does not register an offense when using anonymous `<=`' do
expect_no_offenses(<<~RUBY)
Model.where('column <= ?', value)
RUBY
end

it 'does not register an offense when using anonymous `<`' do
expect_no_offenses(<<~RUBY)
Model.where('column < ?', value)
RUBY
end
Copy link
Member

Choose a reason for hiding this comment

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

Prism matrix is failure. Can you use unsupported_on: :prism?
https://github.com/rubocop/rubocop-rails/actions/runs/8860610072/job/24331577196?pr=1272

Suggested change
it 'does not register an offense when using anonymous `<=`' do
expect_no_offenses(<<~RUBY)
Model.where('column <= ?', value)
RUBY
end
it 'does not register an offense when using anonymous `<`' do
expect_no_offenses(<<~RUBY)
Model.where('column < ?', value)
RUBY
end
context 'TargetRubyVersion <= 2.6', :ruby26, unsupported_on: :prism do
it 'does not register an offense when using anonymous `<=`' do
expect_no_offenses(<<~RUBY)
Model.where('column <= ?', value)
RUBY
end
it 'does not register an offense when using anonymous `<`' do
expect_no_offenses(<<~RUBY)
Model.where('column < ?', value)
RUBY
end
end

# column <[=] :value
LTEQ_NAMED_RE = /\A([\w.]+)\s+(<=?)\s+:(\w+)\z/.freeze
# column >= :value1 AND column <[=] :value2
RANGE_NAMED_RE = /\A([\w.]+)\s+>=\s+:(\w+)\s+AND\s+\1\s+(<=?)\s+:(\w+)\z/i.freeze
Copy link
Member

Choose a reason for hiding this comment

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

Can these constants be moved above minimum_target_ruby_version?

@fatkodima
Copy link
Contributor Author

Made updates.

@koic koic merged commit 7e691de into rubocop:master May 2, 2024
14 checks passed
@koic
Copy link
Member

koic commented May 2, 2024

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.

Cop idea: Rails/WhereBetween
3 participants