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

use model name to determine the type #953

Merged
merged 1 commit into from
Jun 13, 2015

Conversation

lsylvester
Copy link
Contributor

I propose changing the Serializer#type from being based on the classes model_name instead of being based on the class itself.

This provides two advantages:

  • It allows the type to be customized at the model layer (currently can only be done at the serializer layer), and the type will be consistent with the other places that rails uses names (like routes).
  • The model_name memoized so the underscoring and pluralization are only done once, so it should be faster.

It changes the behaviour of namespaced models (was previously demodulized), but the introduction of the demodulization was to change it from spam/unrelated_links. I don't know if there is any reason that unrelated_links would be preferable to spam_unrelated_links

@joaomdmoura
Copy link
Member

@lsylvester Thank you for this PR.
I'm okay with the proposal, it makes sense to me. @kurko any opinions about it? I checked json api docs and it still is consistent to it.
Could you please rebase it with master to fix the conflicts?

@lsylvester lsylvester force-pushed the use-active-model-name-for-type branch from a9f9c95 to 97e82c3 Compare June 13, 2015 09:45
@lsylvester
Copy link
Contributor Author

@joaomdmoura I have rebased this against master

@kurko
Copy link
Member

kurko commented Jun 13, 2015

👍 I like it.

kurko added a commit that referenced this pull request Jun 13, 2015
@kurko kurko merged commit 7fa123b into rails-api:master Jun 13, 2015
@fbernier
Copy link

I like the idea but see a problem with this implementation. Currently, it's quite easy to use AMS outside of active_model by simply including ActiveModel::Serialization into an object, for example: Hashie::Mashie wrapping an Elasticsearch result.

I haven't tested if this commit breaks anything but model_name is active_model specific and might make it more complicated to use outside of active_model.

Is this really what you guys want?

@joaomdmoura
Copy link
Member

@fbernier indeed, this is a good point.

As AMS is becoming part of Rails itself it's hard to deny that it's deeply associated with ActiveModel, even our tests are all focused into models, but in the other hand I'm really happy that people are able to use AMS in unusual ways 😄

It seems that this PR itself isn't breaking the usage of AMS as long as you are serializing an ActiveRecord Object, as we are calling model_name into the object instance that is being serialized.
This isn't the only dependency of AR, a lot of other methods into AMS expect the object being serialized to be an AR object, so I think that you will probably be good to go. 😄

@kurko
Copy link
Member

kurko commented Jun 15, 2015

@fbernier if this breaks in any way, just create a method called model_name. Perhaps the name model_name could be broader, such as entity_name so that it doesn't feel like you're dealing with Rails on your ES results.

You can also override the serializer#type.

@lsylvester
Copy link
Contributor Author

@fbernier You can add the model_name method to non AR classes by adding extend ActiveModel::Naming

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