-
Notifications
You must be signed in to change notification settings - Fork 982
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
Fixes #12530 - Upgrade rubocop to 0.35.1 #2924
Conversation
10ec96c
to
1ea8cdc
Compare
Are the disabled cops all of the new ones, or just all of the new ones that fail in some way? (Missing offence counts.) It might be preferable to refresh the todo file from --auto-gen-config, so we get to use the new cops for future changes and keep them disabled only where there are existing problems? e.g. cops such as Performance/Detect are valuable. |
These are just the new cops that are failing. In rubocop 0.33.0 they changed --auto-gen-config to exclude files that have problems instead of disabling the cops altogether (see https://github.com/fusor/fusor/pull/465/files). This effectively turns on the cop for any new code. I'm not sure I like that. However, it looks like I can generate the todo file with cops being disabled by using Let me know if you prefer excluding files over disabling outstanding cops. |
1ea8cdc
to
84087cb
Compare
Metrics/AbcSize: | ||
Max: 194 | ||
Max: 114 |
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.
Glad to see we decreased the max since Rubocop was added 😄
@daviddavis Rubocop is complaining about the length of 2 classes (no kidding), I'd say 👍 after fixing that. |
84087cb
to
2e2d9b9
Compare
@dLobatog thanks. Should be fixed now. |
Yes, I did prefer the exclusion of files, so we get the benefit of some cops on new code - though I guess I was particularly interested in the performance ones, less that we should enforce different style. Should we make that distinction? |
2e2d9b9
to
badf50e
Compare
@domcleal updated. thanks. |
Merged as a9d8cde, thanks @daviddavis. |
This is mostly because of improved problem detection. A couple cops have bugs that cause false positives and negatives. There are also some new cops I'd like to enable like
Lint/DuplicateMethods
but will do so after we upgrade to Rails 4