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

Closed
wants to merge 2 commits into
from

Conversation

Projects
None yet
4 participants
@jch
Contributor

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 referenced this pull request Feb 13, 2013

Closed

Mash and object_id #73

@jch

This comment has been minimized.

Show comment
Hide comment
@jch

jch Feb 13, 2013

Contributor

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

Contributor

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

This comment has been minimized.

Show comment
Hide comment
@mbleigh

mbleigh Feb 13, 2013

Member

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?

Member

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

This comment has been minimized.

Show comment
Hide comment
@jch

jch Feb 13, 2013

Contributor

Cool with id and type. What about object_id?

Contributor

jch commented Feb 13, 2013

Cool with id and type. What about object_id?

@mbleigh

This comment has been minimized.

Show comment
Hide comment
@mbleigh

mbleigh Feb 13, 2013

Member

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

Member

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

This comment has been minimized.

Show comment
Hide comment
@jch

jch Feb 13, 2013

Contributor

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.

Contributor

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 Feb 13, 2013

@skipchris

This comment has been minimized.

Show comment
Hide comment
@skipchris

skipchris Feb 18, 2013

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: redefiningobject_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?

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: redefiningobject_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

This comment has been minimized.

Show comment
Hide comment
@jch

jch Feb 18, 2013

Contributor

@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?

Contributor

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

This comment has been minimized.

Show comment
Hide comment
@skipchris

skipchris Feb 18, 2013

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

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

Remove override of object_id #77
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

This comment has been minimized.

Show comment
Hide comment
@matschaffer

matschaffer Feb 18, 2013

Contributor

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

Contributor

matschaffer commented Feb 18, 2013

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

jch added a commit that referenced this pull request Feb 18, 2013

@skipchris

This comment has been minimized.

Show comment
Hide comment

:D

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment