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
[Fix #8950] Add IgnoredMethods
and IgnoredClasses
to Lint/NumberConversion
#8956
Conversation
be7876d
to
3691c3e
Compare
I think this approach is fine. For #8950, default.yml of rubocop-rails (or .rubocop.yml of user) can the following settings: Lint/NumberConversion:
IgnoredMethods:
- seconds
- second
- hours
- hour
(snip) And the following methods can be added. |
@@ -0,0 +1 @@ | |||
* [#8950](https://github.com/rubocop-hq/rubocop/pull/8950): Add `IgnoredMethods` and `IgnoredClasses` to `Lint/NumberConversion`. ([@dvandersluis][]) |
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.
* [#8950](https://github.com/rubocop-hq/rubocop/pull/8950): Add `IgnoredMethods` and `IgnoredClasses` to `Lint/NumberConversion`. ([@dvandersluis][]) | |
* [#8950](https://github.com/rubocop-hq/rubocop/issues/8950): Add `IgnoredMethods` and `IgnoredClasses` to `Lint/NumberConversion`. ([@dvandersluis][]) |
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'll fix this but FYI this is coming from the rake changelog:*
tasks. I assume it's done like this because it's impossible to tell whether the reference is an issue or PR, but github redirects pull/xxx
to issues/xxx
if it's actually an issue so I assume that's why @marcandre did it this way.
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.
Agree it doesn't actually matter. Still, I'll change the script so it writes issues/nnn
when there is [Fix #nnn]
in the commit message, and pulls/x
if there isn't (assuming that the link will point to the PR to be created). #8963
Oh nice great suggestion @koic, I’ll make a PR to rubocop-rails tomorrow! |
It'd be nice if we updated the cop description with explanation of its limitations and why we had to disable it by default. |
…/NumberConversion`. - Updated the hardcoded classes to ignore to be configurable instead
3691c3e
to
be76c44
Compare
@bbatsov added to the cop description. Should I make it not use a base with |
I guess that would miss all cases where the first argument is not a literal, so probably we're better off not doing so, at least for now. Thanks for tackling this! |
Fixes #8950, to an extent. There are going to inherently be a number of false positives possible from this cop since it applies to basically any receiver of
to_i
, etc. other thanTime
andDateTime
, which were hardcoded. Since this cop is disabled by default we probably don't have a lot of external coverage here (searching github for Lint/NumberConversion returns a lot of inline disables though!)Instead, the classes to ignore are now configurable, and
IgnoredMethods
was added as well (so the rails duration helpers could be allowed in configuration). This of course isn't going to help when assigned to a variable, because those are checked as well:Another potential issue is that
Integer(10.minutes)
will work butInteger(10.minutes, 10)
, which is the default suggestion/correction, does not work when the first argument is not a string. Should I change that as well to only add the base if the first arg is a known string?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 RuboCop for itself, and generates the documentation.