-
-
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 autocorrection for even? and odd? style cop. #1506
Conversation
@@ -14,32 +14,52 @@ module Style | |||
# # good | |||
# if x.even? | |||
class EvenOdd < Cop | |||
MSG_EVEN = 'Replace with `Fixnum#even?`.' | |||
MSG_ODD = 'Replace with `Fixnum#odd?`.' | |||
MSG = { |
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.
Or
MSG = 'Replace with `Fixnum#%s?`.'
That's a common pattern in other cops.
@jonas054 I updated the pr. I didn't squash so you could see the changes. Let me know if you like them and I'll squash it for merge. |
private | ||
|
||
def variable_name(node) | ||
node.loc.expression.source.match(/\(?(.*?) ?%/)[1] |
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 a regexp here is error-prone. The expression could, for example, start with multiple (
. It's better to use the same way of getting the contents of the parentheses as we do in div_by_2?
(and extract that bit into its own method).
@jonas054 good catch with the regex, I thought I could cheat lol I've updated the pr and added a few more tests. |
private | ||
|
||
def base_number(node) | ||
node = expression(node.children.first) |
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.
We usually avoid the children
attribute in favor of unpacking, e.g. receiver, _method, _args = *node
.
I just had one comment. But overall it looks fine now, I think. |
Pushed and squashed. |
Coverage decreased (-0.03%) when pulling c397d30167ad1be32dc2e2915d8bf2bf4ce95eb8 on blainesch:feature/even-odd into 485531a on bbatsov:master. |
Maybe I'm new to coveralls, but it looks like it's running all the code: Maybe somebody could explain it to me? |
|
||
ZERO = s(:int, 0) | ||
ONE = s(:int, 1) | ||
TWO = s(:int, 2) | ||
|
||
def on_send(node) | ||
violation = violation_type(node) |
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.
We use only the term offense
throughout the codebase.
Rebased. |
Add autocorrection for even? and odd? style cop.
👍 |
Just noticed you forgot to add a changelog entry. Guess you can do so in a separate PR. |
No description provided.