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/DefaultScope cop #267

Merged
merged 1 commit into from
Jul 13, 2020
Merged

Conversation

fatkodima
Copy link
Contributor

Closes #76

Personally, I agree with everything stated in that issue and in numerous blog posts (including referenced in Reference), and do not find default_scope as useful, but as a time bomb and a source of bugs to be explored in production.

CI does not pass because there was a cop named DefaultScope in the past (but which did absolutely different thing, just checked default_scope style - lambda vs block).
https://github.com/rubocop-hq/rubocop/blob/423323cc7f5af33cd2123bfcadb8eec4da4e0d5e/lib/rubocop/config_obsoletion.rb#L66

So should that line be removed, or this cop renamed, or something else?

@bbatsov
Copy link
Contributor

bbatsov commented Jun 25, 2020

This was a long time ago, so I'd remove that line.

config/default.yml Outdated Show resolved Hide resolved
config/default.yml Outdated Show resolved Hide resolved
# end
#
class DefaultScope < Cop
MSG = 'Avoid using `default_scope`. It is better to use explicitly named scopes.'
Copy link
Contributor

@andyw8 andyw8 Jun 26, 2020

Choose a reason for hiding this comment

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

I wonder if this can be phrased to indicate that using a named scope is not the only alternative. For example, hidden records could be moved to a separate table.

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 think, most of the time the solution is just to name a scope. I'm fine with message, but maybe it is better to just remove the second sentence.

@Drenmi
Copy link
Contributor

Drenmi commented Jun 26, 2020

A convention I've come across a number of times is to always put default_scope at the top of the class to make it more discoverable. (After any includes.) Maybe this could be a configuration option?

@fatkodima
Copy link
Contributor Author

@Drenmi
Albeit related to default_scope, but this cop is about a little different thing.
Maybe it is better to introduce new (or extend existing Layout/ClassStructure cop) to handle this and related?

config/default.yml Outdated Show resolved Hide resolved
lib/rubocop/cop/rails/default_scope.rb Outdated Show resolved Hide resolved
lib/rubocop/cop/rails/default_scope.rb Outdated Show resolved Hide resolved
docs/modules/ROOT/pages/cops_rails.adoc Outdated Show resolved Hide resolved
@fatkodima fatkodima force-pushed the default-scope-cop branch 2 times, most recently from 58bddb0 to cdfa9fc Compare July 6, 2020 10:39
@koic koic merged commit e3754a0 into rubocop:master Jul 13, 2020
@koic
Copy link
Member

koic commented Jul 13, 2020

Thanks!

koic added a commit to koic/rubocop-rails that referenced this pull request Jul 15, 2020
@koic koic mentioned this pull request Jul 15, 2020
9 tasks
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 against default_scope
6 participants