Skip to content

Conversation

mhenrixon
Copy link
Contributor

This is still sort of a hack. I don't like what I have to do to be able to camelize the root key, I believe a simple setting to switch on would have been nicer but hey it works.

Any suggestions for improvements deeply appreciated.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.03%) when pulling d0e0e1b on mhenrixon:convert_keys into ebe3afe on rails-api:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.03%) when pulling 3c362b4 on mhenrixon:convert_keys into ebe3afe on rails-api:master.

@mhenrixon mhenrixon mentioned this pull request Oct 27, 2013
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this needed? ^^^

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not really, I can do def self.included if you want?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You should just include Convertable, there's no need to use that hook

@spastorino
Copy link
Contributor

All the things changed in the README file does not belong to this commit.
And were all already pushed in a previous PR.

@spastorino
Copy link
Contributor

Last thing is that I'd execute the convertion of the keys right after the execution of attributes, has_one and has_many so we don't pay the penalty of converting these things on each request.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We are discussing about how to treat keys but meanwhile we don't reach a conclusion I wouldn't convert the key back to symbol.
Just leave it as is.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't like the string key return at all, that's my personal opinion and for what it is worth you can take that into consideration when discussing it :) I'll change it for now though I would hate to see AMS get all stringy on me. What would the benefit of that be?

@spastorino
Copy link
Contributor

Thanks a lot for your work.

@mhenrixon
Copy link
Contributor Author

Last thing is that I'd execute the convertion of the keys right after the execution of attributes, has_one and has_many so we don't pay the penalty of converting these things on each request.

The problem is it needs to be done for roots as well. Right now we have some logic in serializable.rb and some in serializer.rb. I still want to convert the root element key right?

@mhenrixon
Copy link
Contributor Author

Regarding when to convert keys:

def attributes(*attrs)
  @_attributes.concat attrs
  attrs.each do |attr|
    define_method attr do
      object.read_attribute_for_serialization attr
    end unless method_defined?(attr)
  end
end

Won't that I have to keep track of both the converted key and the original key? Is the penalty really worth it?

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.29%) when pulling 7f32e16 on mhenrixon:convert_keys into ebe3afe on rails-api:master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.3%) when pulling 8583cb2 on mhenrixon:convert_keys into 49ab359 on rails-api:master.

@mhenrixon
Copy link
Contributor Author

Closing in favor of #436 since that seems like a better solution to me.

@mhenrixon mhenrixon closed this Nov 1, 2013
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.

4 participants