-
-
Notifications
You must be signed in to change notification settings - Fork 263
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
Check for or
method in Rails/FindEach
cop
#995
Conversation
936eba0
to
5d79903
Compare
lib/rubocop/cop/rails/find_each.rb
Outdated
all eager_load includes joins left_joins left_outer_joins not preload | ||
references unscoped where | ||
references unscoped where or |
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.
Can you update to order by ASCII position?
- all eager_load includes joins left_joins left_outer_joins not preload
- references unscoped where or
+ all eager_load includes joins left_joins left_outer_joins not or preload
+ references unscoped where
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.
I fixed it!
@@ -33,6 +33,7 @@ | |||
it_behaves_like('register_offense', 'unscoped') | |||
it_behaves_like('register_offense', 'where(name: name)') | |||
it_behaves_like('register_offense', 'where.not(name: name)') | |||
it_behaves_like('register_offense', 'where(name: name).or(User.where(age: age))') |
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.
Can you move to between L30 (left_outer_joins
) and L31 (preload
)?
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.
I fixed it!.
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.
https://github.com/rubocop/rubocop-rails/pull/995/files#diff-8b9e38f28a7bdb108efab88f1f3495067c22717be7973a6c73e92d5b0f599287L35
If you want to maintain order defined in SCOPE_METHODS
, should I move this between left_outer_joins
and or
?
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.
Let's leave the existing one in favor of keeping git blame
. Thank you.
5d79903
to
bf38577
Compare
Thanks! |
This PR adds
or
method to the list of check methods inRails/FindEach
cop. Since the cop already checks for the use ofwhere
andnot
, I thought it would be better to includeor
as well.Before submitting the PR make sure the following are checked:
[Fix #issue-number]
(if the related issue exists).master
(if not - rebase it).bundle exec rake default
. It executes all tests and runs RuboCop on its own code.{change_type}_{change_description}.md
if the new code introduces user-observable changes. See changelog entry format for details.