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

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

Merged
merged 1 commit into from
May 25, 2012
Merged

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

merged 1 commit into from
May 25, 2012

Conversation

ayamomiji
Copy link
Contributor

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

@josevalim
Copy link
Contributor

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

@spastorino
Copy link
Contributor

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

@josevalim
Copy link
Contributor

Yeah, probably :P

@spastorino
Copy link
Contributor

@ayamomiji please do it and squash your commits :)

@ayamomiji
Copy link
Contributor Author

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
Copy link
Contributor Author

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
Copy link
Contributor

parndt commented Mar 12, 2012

What's happening here? :)

@isaacsanders
Copy link
Contributor

Is this still an issue?

@spastorino
Copy link
Contributor

/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
Copy link
Contributor Author

+1 for always do send.

@isaacsanders
Copy link
Contributor

👎 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
Copy link
Contributor

@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
Copy link
Contributor

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
Copy link
Contributor

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

@carlosantoniodasilva
Copy link
Member

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

@spastorino
Copy link
Contributor

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

@josevalim
Copy link
Contributor

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
Copy link
Member

@josevalim are we good to go now? :)

josevalim added a commit that referenced this pull request May 25, 2012
…-using-ruby-keyword

add `self.` to allow method name using ruby keyword
@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
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants