Add configuration with camelizing keys #160
Conversation
| @@ -0,0 +1,30 @@ | |||
| # frozen_string_literal: true | |||
There was a problem hiding this comment.
We should have documentation for this, can you please add?
If not, I'll add after this PR will be merged.
There was a problem hiding this comment.
there is small documentation with example in lib/panko_serializer.rb how to use it
but probably you can add any documentation later
There was a problem hiding this comment.
I was talking about the documentation here: https://github.com/yosiat/panko_serializer/tree/master/docs
| Attribute.new(name.to_s, alias_name) | ||
| end | ||
|
|
||
| def self.transform_key(name) |
There was a problem hiding this comment.
can we handle the case where key_type is nil and just return the name as is?
There was a problem hiding this comment.
and looks like I have added check before method calling if Panko.configuration.key_type, but I'll add in any case
There was a problem hiding this comment.
and I think about memoization at this point, what do you think?
if developer serializes 1000 elements with the same key, no need to transform key 1000 times, 1 is enough
There was a problem hiding this comment.
- Why we need the
if Panko.configuration.key_typeinself.create, we already handle intransform_keynow. and I think about memoization at this point, what do you think?- I don't think it's necessary, since attributes are creating when serializer is defined and not during serialization time, so you pay the "transformation cost" only at boot time.
lib/panko/attribute.rb
Outdated
| @@ -4,9 +4,17 @@ module Panko | |||
| class Attribute | |||
| def self.create(name, alias_name: nil) | |||
| alias_name = alias_name.to_s unless alias_name.nil? | |||
| alias_name = transform_key(name.to_s) if alias_name.nil? && Panko.configuration.key_type | |||
There was a problem hiding this comment.
I'm not entirely sure what the expected behavior should be when an alias is provided in snake case, but the configured key type is in camel case. What do you think? My suggestion is to camelize the alias if it's provided, but I'd like to hear your thoughts.
There was a problem hiding this comment.
I see your point, I think, while transforming keys is general setting for all keys and aliases is specific setting, then aliases should be in priority, like developer has general settings with transforming, but for specific key he can specify alias
and if you agree with this - then no changes is required in PR
There was a problem hiding this comment.
Agree with you, so please:
- Add test for this case, to make sure we don't change the alias
- In the documentation (https://github.com/yosiat/panko_serializer/pull/160/files#r1785809763) - we should note that (if you won't write the doc, I'll mention it..)
There was a problem hiding this comment.
I added test for using configuration with alias
Co-authored-by: Yosi Attias <[email protected]>
| Attribute.new(name.to_s, alias_name) | ||
| end | ||
|
|
||
| def self.transform_key(name) |
There was a problem hiding this comment.
- Why we need the
if Panko.configuration.key_typeinself.create, we already handle intransform_keynow. and I think about memoization at this point, what do you think?- I don't think it's necessary, since attributes are creating when serializer is defined and not during serialization time, so you pay the "transformation cost" only at boot time.
lib/panko/attribute.rb
Outdated
| @@ -4,9 +4,17 @@ module Panko | |||
| class Attribute | |||
| def self.create(name, alias_name: nil) | |||
| alias_name = alias_name.to_s unless alias_name.nil? | |||
| alias_name = transform_key(name.to_s) if alias_name.nil? && Panko.configuration.key_type | |||
There was a problem hiding this comment.
Agree with you, so please:
- Add test for this case, to make sure we don't change the alias
- In the documentation (https://github.com/yosiat/panko_serializer/pull/160/files#r1785809763) - we should note that (if you won't write the doc, I'll mention it..)
@yosiat I made last changes, maybe it's better for you to change documentation in your way |
close #52
to camelize keys rails application can have initializer
@yosiat what do you think?