Namespace separator setting for json-api and tests #1874
Conversation
|
@NullVoxPopuli, thanks for your PR! By analyzing the annotation information on this pull request, we identified @dubadub, @groyoh and @remear to be potential reviewers |
| - [#1791](https://github.com/rails-api/active_model_serializers/pull/1791) (@bf4, @NullVoxPopuli) | ||
| - JSON API Type Transform functions independent of Key Transform. | ||
| - Added jsonapi_type_transform config option. | ||
| - Added jsonapi_namespace_separator config option. |
There was a problem hiding this comment.
Both, I think.
and that key_transform won't affect the type's key.
The new options could be configured to behave the old way, I'm pretty sure
|
tests pass! |
| # transform('SomeClass', :underscore) => 'some_class' | ||
| # transform('some_class', :snake) => 'someClass' | ||
| # etc... | ||
| def transform(string, type) |
There was a problem hiding this comment.
I recall asking this in an earlier revision but I'll ask again. Why is this transform method needed? Why not do it like
There was a problem hiding this comment.
that's a good idea! I'll do that! standby
There was a problem hiding this comment.
I think it's legacy from the original impl?
There was a problem hiding this comment.
wait... isn't that method you point to implemented by this method?
require 'active_model_serializers/key_transform'
KeyTransform.send(transform(options), value)There was a problem hiding this comment.
When I originally wrote this module it did not include the transform method. https://github.com/rails-api/active_model_serializers/blob/3498647d1a07b9e8a183702bef53cf1ce8a76370/lib/active_model_serializers/key_transform.rb
It seems to have appeared in 913f396
There was a problem hiding this comment.
removed. I was confused about which transform method we were talking.
I need to stop coding when I'm tired...
:embarassed:
documentation Type transform setting and transformer, name fixed in tests
bf4
force-pushed
the
namespace_separator_setting
branch
from
becdf10 to
b5b4915
Compare
|
@NullVoxPopuli @youroff Updated PR to just be the namespace transform |
|
(Didn't benchmark..) |
|
Sweet! now it's no longer a breaking change! :-) |
|
Also added a few learnings about serializer coupling from #1857 |
| @@ -2,11 +2,30 @@ module ActiveModelSerializers | |||
| module Adapter | |||
| class JsonApi | |||
| class ResourceIdentifier | |||
| def self.type_for(class_name, serializer_type = nil, transform_options = {}) | |||
There was a problem hiding this comment.
reason for making a class method: tied too much logic to serializer instances make refactoring difficult, especially when, as demonstrated here, the actual serializer instance is not necessary for the transformation
see problems in #1857 :)
There was a problem hiding this comment.
welcomed change!
|
Good to merge? Looks like there should be two output 'ready for pr' issues |
|
@bf4 I'd say so. Nice work! |
@bf4 resolved #1609,
merged into master, and would like @remear and @youroff to review for any
changes, since I'm merging this after changes in the KeyTransform code were made.
based on: #1791