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
Updated channel and documentation to Rubocop v0.80.0 #225
Updated channel and documentation to Rubocop v0.80.0 #225
Conversation
If a cop's documentation grows too long (e.g. Style/BlockDelimiters), it will run afoul of the Metrics/ClassLength cop, which will need to be disabled. This breaks the parsing. Adjusting the initial gsub's regexp pattern appears to resolve this edge-case.
I'm not sure what the desired outcome here is, actually.
Style/HashEachMethods: | ||
Enabled: false | ||
Style/HashTransformKeys: | ||
Enabled: false | ||
Style/HashTransformValues: | ||
Enabled: false |
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.
While this change does cause the spec to pass, I'm not sure it's right way to handle the stdout warning about new cops.
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 would say let's turn them ON!
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.
Your wish is my command! 51b5aa4
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.
Makes sense to enable them since they are enabled by default in Rubocop.
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.
Note that they are not enabled by default in RuboCop, but rather "pending" which means the users have to decide whether to enable them or not. We'll likely enable them by default in version 1.0.
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.
Ok, lets leave it on in the spec, but keeping it out of the default config, so it warns users to make a decision, just like rubocop itself.
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.
Ok, lets leave it on in the spec, but keeping it out of the default config, so it warns users to make a decision, just like rubocop itself.
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.
Done in a81a64e. ✅
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.
Github is not helping much today... seems to be having issues.
I've tried to merge this early today and seems like the branch got into a weird state instead.
... just checked, it actually merged the changes in, but kept the PR open! So weird.
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.
The diff is now weird, but your changes are there.
I will be releasing this soon.
I should also note that the RSpec suite does run, but with some additional noise in the output from the following Rubocop warning:
|
@DanielWright that's expected due to rubocop/rubocop#7567 |
Is it desirable to handle or suppress the warnings in this PR? |
I guess the Code Climate team need to make a decision here on whether upgrading the channel should make the newer cops available by default or not. |
Per feedback from @filipesperandio, the config upgrader spec will now exercise the upgrades with the new cops enabled, rather than disabled.
No concerns here. As state earlier, we switched to a policy where RuboCop will enable cops only at major releases and the rest of the time it'd be up to users to decide if they want to enable/disable some new cop. I view the message you mentioned as purely informational, not a warning. :-) |
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.
Looks good
# bad | ||
sig do | ||
params( | ||
foo: string, |
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.
Trailing comma, is this a deliberate choice?
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.
This appears to be a choice made upstream, whether a typo or deliberate I cannot say.
* Updated channel and documentation to Rubocop v0.79.0 (codeclimate#222) * Bumps 'rubocop' Dependency to v0.80.0 * Updates Scraped Documentation * Handles Edge-Case in Documentation Parsing Logic If a cop's documentation grows too long (e.g. Style/BlockDelimiters), it will run afoul of the Metrics/ClassLength cop, which will need to be disabled. This breaks the parsing. Adjusting the initial gsub's regexp pattern appears to resolve this edge-case. * Minor Delinting * Fixes Namespace Warnings in Config Files * Possible Fix for Config Upgrader Spec I'm not sure what the desired outcome here is, actually. * Updates ConfigUpgrader Spec Per feedback from @filipesperandio, the config upgrader spec will now exercise the upgrades with the new cops enabled, rather than disabled. Co-authored-by: Filipe Esperandio <filipesperandio@gmail.com>
Most of this PR is pretty straightforward, and I'll leave some PR comments where I've got questions. At a minimum, this branch should provide a solid base for updating the channel.