Add configuration with camelizing keys - #160
Conversation
yosiat
left a comment
yosiat
left a comment
There was a problem hiding this comment.
Thanks for contributing this!
Highly appreciated and looks good, suggesting few changes.
| @@ -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.
| 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 <yosy101@gmail.com>
yosiat
left a comment
yosiat
left a comment
There was a problem hiding this comment.
This will be the last round of the CR.
Thanks again for your contribution.
| 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.
| 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 |
|
Hello, this PR seems to be stale, which is a big waste. Is possible to push the last mile? 🙏🏻 Can I help with that? |
3 participants
close #52
to camelize keys rails application can have initializer
@yosiat what do you think?