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

Pass :x instead of block in proc leads to an error #1485

Closed
vmeyet opened this issue Dec 10, 2014 · 4 comments
Closed

Pass :x instead of block in proc leads to an error #1485

vmeyet opened this issue Dec 10, 2014 · 4 comments

Comments

@vmeyet
Copy link

vmeyet commented Dec 10, 2014

Opening an issue for the same reason as the comment of @monfresh at the end of that issue Pass &:x? as an argument to lambda instead of a block..

here is the comment he made:

I think this is also a bug for procs. When I have this:

validates :address,
          presence: {
            message: I18n.t('errors.messages.no_address')
          },
          unless: proc { |location| location.virtual? }
rubocop complains and says:

Pass &:virtual? as an argument to proc instead of a block.
If I change the code to this:

unless: proc(&:virtual?)
Then I get Argument Error: no receiver during validation.

I got the very same issue:

# In a rails activemodel
after_save :geocode_and_add_city, :if => proc { |loc| loc.address_changed? }

which is flagged by rubocop (offense Style/SymbolProc)

app/models/location.rb:17:46: C: Pass &:address_changed? as an argument to proc instead of a block.
    after_save :geocode_and_add_city, :if => proc { |l| l.address_changed? }

but if changed to proc(&:address_changed?) correcting the offense then the code fails: ArgumentError: no receiver given. Basically it see the proc as taking no argument (especially not the instance) which led to the error.

This should probably be ignored by rubocop.

@bbatsov
Copy link
Collaborator

bbatsov commented Dec 10, 2014

proc { |location| location.virtual? }

Btw, I didn't think much about this last time, but why is it a problem? After all:

p = proc(&:upcase)
p.call("hello")
# => "HELLO"

@vmeyet
Copy link
Author

vmeyet commented Dec 10, 2014

For sure it make sense, however there is no control on how proc is called (rails internal).

In short the call is made by the active_suport gem:
in lib/active_support/callbacks.rb

          if filter.arity <= 0
            lambda { |target, _| target.instance_exec(&filter) }
          else
            lambda { |target, _| target.instance_exec(target, &filter) }
          end

(filter being the proc)

hence the arity when doing proc(&:upcase) is -1 and the instance_exec do not use the target (which is the active record instance)

p = proc(&:upcase)
p.arity
#=> -1

when doing proc {|loc| loc.virtual?} the arity is 1 and so it works fine

p = proc {|loc| loc.virtual?}
p.arity
#=> 1

@bbatsov
Copy link
Collaborator

bbatsov commented Dec 10, 2014

I'll make the necessary change, as I already made one for lambda, but I'd suggest raising an issue on the Rails tracker about this. They should try to support such lambdas/procs.

Generally I don't want us to have to handle Rails specific stuff in a tool that's generic.

@vmeyet
Copy link
Author

vmeyet commented Dec 10, 2014

Thanks for that,

After checking rails internals, and testing stuff around (with the idea to open a rails issue), it seems like doing:

after_save :geocode_and_add_city, :if => :address_changed?

instead of

after_save :geocode_and_add_city, :if => proc { |loc| loc.address_changed? }

does work

Not that it changes anything (the correction given by rubocop still fails), but it is interesting to note. Given that I don't think rails will change anything to it's implementation.

Anyway thanks for reacting so promptly!

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

No branches or pull requests

2 participants