add `self.` to allow method name using ruby keyword #4785

Merged
merged 1 commit into from May 25, 2012

Projects

None yet

6 participants

@ayamomiji

fix alias_attribute will raise a syntax error if aliasing a column named as a ruby keyword.

@josevalim
Member

Can we add a test for at least 1 of those keywords?

@spastorino
Member

Is not enough to just prepend self always instead of checking against RUBY_KEYWORDS ?

@josevalim
Member

Yeah, probably :P

@spastorino
Member

@ayamomiji please do it and squash your commits :)

@ayamomiji

But i cannot just prepend self. always, this causes the DirtyTest to raise errors like these:

test_setting_attribute_will_result_in_change(DirtyTest):
NoMethodError: private method `attribute_changed?' called for #<DirtyTest::DirtyModel:0x007faa6b038b40>
    /Users/ayaya/Projects/rails/activemodel/lib/active_model/attribute_methods.rb:373:in `method_missing'
    /Users/ayaya/Projects/rails/activemodel/lib/active_model/attribute_methods.rb:314:in `name_changed?'
...

NoMethodError: private method `attribute_will_change!' called for #<DirtyTest::DirtyModel:0x007faa6b057568 @name=nil, @color=nil>
    /Users/ayaya/Projects/rails/activemodel/lib/active_model/attribute_methods.rb:373:in `method_missing'
    /Users/ayaya/Projects/rails/activemodel/lib/active_model/attribute_methods.rb:314:in `color_will_change!'
...
@ayamomiji

i'll use send instead of prepending self. for prevent make aliases on private attributes.
i'm not sure should attributes always be public?

@parndt
parndt commented Mar 12, 2012

What's happening here? :)

@isaacsanders

Is this still an issue?

@spastorino
Member

/cc @josevalim I don't see another way of doing this but I don't like it neither :/. Or we leave the Ruby keywords list or we remove the CALL_COMPILABLE_REGEXP optimization and always do send(:'#{send}', #{extra})

@ayamomiji

+1 for always do send.

@isaacsanders

👎 I think this is just hacky. :( If they are reserved, we shouldn't be messing with it. We should do make behavior as predictable as possible. In other languages, users have to respect the reserved words. I think we should too. Just my $0.02

@spastorino
Member

@isaacsanders disagree, you can define a method called begin for your classes in pure Ruby. So why stopping people for doing that on AM attributes?

@isaacsanders

I don't know. It is just my opinion. I feel bad that I can't vocalize why I feel the way I do.

@spastorino
Member

@isaacsanders no worries :), I was giving my opinion too ❤️ ❤️ ❤️

@carlosantoniodasilva

@spastorino @josevalim are we good to go here? :)

@spastorino
Member

@carlosantoniodasilva read my comment where I've /cced to @josevalim, let's wait for him

@josevalim
Member

We should just use self. The reason self. is making dirty tests fail is because your changes are affecting a place it should not to. There are two methods in the attributes module that calls define_optimized_call, one is alias attribute and there is a second one. You are mistakenly affecting the second one. Here is a patch for example that makes your tests pass:

https://gist.github.com/2686886

Can you please update accordingly?

@carlosantoniodasilva

@josevalim are we good to go now? :)

@josevalim josevalim merged commit 56417b4 into rails:master May 25, 2012
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment