-
-
Notifications
You must be signed in to change notification settings - Fork 3.1k
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 UnifiedInteger
cop
#3492
Conversation
137c3b8
to
2e6e7c9
Compare
# # good | ||
# 1.is_a?(Integer) | ||
class UnifiedInteger < Cop | ||
MSG = 'Use Integer instead of %s'.freeze |
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.
Wrap the class names in backticks. The message should end with a .
.
|
||
context 'when Integer' do | ||
context 'without any decorations' do | ||
let(:source) { '1.is_a?(Ingeter)' } |
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.
Ingeter -> Integer
end | ||
|
||
context 'when explicitly specified as toplevel constant' do | ||
let(:source) { '1.is_a?(::Ingeter)' } |
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.
Same here.
end | ||
|
||
context 'with MyNamespace' do | ||
let(:source) { '1.is_a?(MyNamespace::Ingeter)' } |
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.
And here.
let(:config) { RuboCop::Config.new } | ||
subject(:cop) { described_class.new(config) } | ||
|
||
%w(Fixnum Bignum).each do |klass| |
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 think using shared examples would be better than generating the tests like this.
You'll also have to rebase this on top of the current |
# | ||
# # good | ||
# 1.is_a?(Integer) | ||
class UnifiedInteger < Cop |
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 wonder if the cop should be enabled for all Ruby versions or just for 2.4+.
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 semantics slightly change starting in 2.4 so it probably makes most sense to only enable for 2.4 https://blog.blockscore.com/new-features-in-ruby-2-4/#simplified-integers
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 don't think so.
In many cases, using Fixnum
is a mistake in Ruby 2.3 or older too. Then we can use Integer
instead of Fixnum
.
I've found the mistake in some projects.
e.g.
# This statement does nothing when x is a Bignum.
case x
when Fixnum # it's should be `Integer`
do_something
when Float
do_something2
end
So, I think this cop shoud be enabled for all Ruby versions. It is useful to detect the mistake, and migration to Ruby 2.4.
df4c5e0
to
8188db9
Compare
I've fixed the message, spec, and typo. and rebased. |
👍 |
New rule introduced in Rubocop 0.43 rubocop/rubocop#3492 Signed-off-by: Rasmus Bergholdt <[email protected]>
Feature
This cop detects using
Fixnum
andBignum
class, e.g.Target Problem
From Ruby 2.4,
Fixnum
andBignum
will be unified toInteger
. https://bugs.ruby-lang.org/issues/12005Fixnum
andBignum
will become a reference ofInteger
from Ruby 2.4, e.g.So, Using Fixnum or Bignum is confusing from Ruby 2.4.
And until Ruby 2.4, the cop is useful for migrating to Ruby 2.4.
See also #3491
Before submitting the PR make sure the following are checked:
[Fix #issue-number]
(if the related issue exists).master
(if not - rebase it).and description in grammatically correct, complete sentences.