convert nested classes in Twitter::Base#attrs #364

Merged
merged 1 commit into from Mar 14, 2013

Conversation

Projects
None yet
2 participants
Collaborator

stve commented Mar 12, 2013

Changes made for #361 resulted in nested classes not being converted to a hash, this calls checks for nested objects when invoking Twitter::Base#attrs

Owner

sferik commented Mar 14, 2013

I'm merging this for now but I may end up reverting this whole concept in a future version.

@sferik sferik added a commit that referenced this pull request Mar 14, 2013

@sferik sferik Merge pull request #364 from sferik/deep_attrs
convert nested classes in Twitter::Base#attrs
e69c928

@sferik sferik merged commit e69c928 into master Mar 14, 2013

1 check passed

default The Travis build passed
Details
Collaborator

stve commented Mar 14, 2013

Seems like either way we decide to go on this, we should probably invoke attrs on nested objects so that the output is a pure hash, no?

Owner

sferik commented Mar 14, 2013

Yup, that's why I merged it and released the patch in version 4.6.1. 😉

I need to think more about the implications of calling reader methods on attribute hash values.

To address your specific objection (methods with default arguments), we could check the method's arity for a non-zero value before calling send. Like this:

 def attrs
   @attrs.inject({}) do |attrs, (key, value)|
     if value.respond_to?(:attrs)
       attrs.merge!(key => value.attrs)
     else
-      attrs.merge!(key => respond_to?(key) ? send(key) : value)
+      attrs.merge!(key => respond_to?(key) && method(key).arity.zero? ? send(key) : value)
     end
   end
 end

I'm not sure whether this is actually necessary, but it's probably not a bad idea, just to be safe.

For what it's worth, version 4.6 has been released for a week and I haven't heard a single complaint about this.

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