Skip to content
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 #4196] Rails/RelativeDate constant complains on lambda and proc #4234

Conversation

smakagon
Copy link
Contributor

@smakagon smakagon commented Apr 3, 2017

Fixes the following case for Rails/RelativeDate cop:

CONST = -> { 1.year.ago }

Before this fix it would show:

bug.rb:1:1: C: Rails/RelativeDateConstant: Do not assign ago to constants as it will be evaluated only once.
CONST = -> { 1.year.ago }

As reported here.

@smakagon smakagon force-pushed the 4196_rails_relative_date_constant_complains_on_lambda_and_proc branch from 16ad660 to 423b79a Compare April 3, 2017 00:07
@smakagon smakagon force-pushed the 4196_rails_relative_date_constant_complains_on_lambda_and_proc branch from 423b79a to 2f09f12 Compare April 3, 2017 00:11
@@ -25,6 +25,8 @@ class RelativeDateConstant < Cop
RELATIVE_DATE_METHODS = %i[ago from_now since until].freeze

def on_casgn(node)
return if node.children.last.lambda_or_proc?
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please, use node destructuring here (e.g. something like _scope, const_name, value = *node) instead of accessing node.children directly. I don't like this, because it gives 0 indication what exactly are you checking when you flatten the nodes to an array.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Btw, it seems to me you can return quickly for anything but a send node on the right-hand side of the assignment. Obviously - if it's not a send it can't be what this cop is checking for.

Overall this cop is implemented in a rather odd manner and can benefit from using node patterns IMO.

@bbatsov bbatsov merged commit 212f68b into rubocop:master Apr 3, 2017
@bbatsov
Copy link
Collaborator

bbatsov commented Apr 3, 2017

I'll merge this because I want to cut a release and I'll fix it myself.

@bbatsov
Copy link
Collaborator

bbatsov commented Apr 3, 2017

Looking more closely in the code - this part bad_node = node.descendants.find { |n| bad_method?(n) } makes no sense to me. We should simply be inspecting whatever's the rhs on this assignment. @smakagon I'd appreciate it if you can take a look at improving this code when you have some time.

@smakagon
Copy link
Contributor Author

smakagon commented Apr 3, 2017

@bbatsov sure, I'll try to refactor that cop and add more specs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants