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 section for redundant all #340

Merged
merged 1 commit into from
Aug 26, 2023
Merged

Conversation

masato-bkn
Copy link
Contributor

Related issue: #339

This PR adds "Redundant all" section.

----
# bad
User.all.order(:created_at)
User.all.find(id)
Copy link
Member

Choose a reason for hiding this comment

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

Is it only applicable to the model or to a relation as well? ‘current_user.likes.all’?

Copy link
Contributor Author

@masato-bkn masato-bkn Aug 11, 2023

Choose a reason for hiding this comment

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

this applies to relation as well.

# bad
current_user.likes.all.order(:created_at)

# good
current_user.likes.order(:created_at)

However, it's uncertain whether this rule can be applied to current_user.likes.all.
In RuboCop, determining whether likes is a relation can be difficult since there are no Active Record query methods following all. If likes is not a relation, removing all might change the result.

Copy link
Member

Choose a reason for hiding this comment

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

True, examples of all method followed by an AR query method could be added.

Copy link
Contributor Author

@masato-bkn masato-bkn Aug 19, 2023

Choose a reason for hiding this comment

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

@koic

True, examples of all method followed by an AR query method could be added.

Sorry for the confusion, but I want to make sure I understand your comment correctly.
Are you suggesting that I should add one or both of the following examples?

(like is a relation of User)

  1. relation.all.ar_query_method
# bad
current_user.likes.all.order(:created_at)

# good
current_user.likes.order(:created_at)
  1. relation.ar_query_method.all
# bad
current_user.likes.order(:created_at).all

# good
current_user.likes.order(:created_at)

Copy link
Member

Choose a reason for hiding this comment

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

I imagined it as 1. Either way, the receiver is an ActiveRecord_Relation object.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for your comment. I've added some examples.

README.adoc Outdated Show resolved Hide resolved
@koic
Copy link
Member

koic commented Aug 21, 2023

@masato-bkn Can you squash your commits into one?

@masato-bkn
Copy link
Contributor Author

masato-bkn commented Aug 21, 2023

@koic
I've squashed the commits!

@koic koic merged commit 2e7d125 into rubocop:master Aug 26, 2023
3 checks passed
@koic
Copy link
Member

koic commented Aug 26, 2023

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.

3 participants