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

Remove special case for id, type, and object_id #77

Closed
wants to merge 2 commits into from

Conversation

jch
Copy link
Contributor

@jch jch commented Feb 13, 2013

Mash overrides the definitions of these core object methods to allow users to
have keys with these names. While it works at first glance, this encourages
bad code that will eventually come back and bite the user. Possible weird
behaviors include:

  • garbage collection object_id
  • object identity comparison
  • object type comparison

I won't merge this into the 1.x series since it's a breaking change, but I
plan to merge this after releasing 1.3

cc @mbleigh

@jch jch mentioned this pull request Feb 13, 2013
@jch
Copy link
Contributor Author

jch commented Feb 13, 2013

On second thought, I don't want to deal with releasing a 1.3 since the last release was so long ago. I'd rather just release 2.0.0

@mbleigh
Copy link
Contributor

mbleigh commented Feb 13, 2013

I'm not sure I entirely agree with this...as unfortunate as it is, id and type are going to be pretty common fields in an API data return (one of Mash's most common use cases). My understanding is that as of Ruby 1.9 the #id method isn't defined on Object (or Hash for that matter). Same goes for type. I'm not really interested in being particularly friendly to Ruby 1.8.x at this point...thoughts?

@jch
Copy link
Contributor Author

jch commented Feb 13, 2013

Cool with id and type. What about object_id?

@mbleigh
Copy link
Contributor

mbleigh commented Feb 13, 2013

I'm ok with object_id requiring a special case since I've never seen that in an API response.

@jch jch closed this Feb 13, 2013
@jch
Copy link
Contributor Author

jch commented Feb 13, 2013

Closing b/c only 1.8 whines about this. There may be a workaround to silence the warning If someone's interested in making a pull, I'd be happy to review it.

@jch jch deleted the remove-special-keys branch February 13, 2013 17:27
@skipchris
Copy link

Hi guys,

Sorry if I've misunderstood the comment that "only 1.8 whines about this", but running our test suite under 1.9.3 I'm seeing:

/Users/chris/.rvm/gems/ruby-1.9.3-p194@sage_one/gems/hashie-2.0.0/lib/hashie/mash.rb:80: warning: redefining object_id' may cause serious problems`

If this is the case (and I'm not being stupid - feel free to correct me if I am), does it make sense to just merge a version of this PR which just removes object_id but leaves id and type?

@jch
Copy link
Contributor Author

jch commented Feb 18, 2013

@skipchris that sounds reasonable to me. If someone does want to override object_id, it'd be easy to do so by reopening the class or making their own subclass. Want to send up a quick pull?

@skipchris
Copy link

Yup, will do - just wanted to check I wasn't missing something!

matschaffer added a commit to matschaffer/hashie that referenced this pull request Feb 18, 2013
Avoids this warning on 1.9.3:

    /Users/mat/.rvm/gems/ruby-1.9.3-p194@.../gems/hashie-2.0.0/lib/hashie/mash.rb:80: warning: redefining `object_id' may cause serious problems
@matschaffer
Copy link
Contributor

@skipchris @jch feel free to use #81 if it helps.

jch added a commit that referenced this pull request Feb 18, 2013
@skipchris
Copy link

:D

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

4 participants