-
Notifications
You must be signed in to change notification settings - Fork 35
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
Add configuration with camelizing keys #160
base: master
Are you sure you want to change the base?
Changes from 4 commits
25d9bda
0750734
cae6566
5de2c2d
eb0f81d
2771ee9
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -4,9 +4,18 @@ 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 | ||
Attribute.new(name.to_s, alias_name) | ||
end | ||
|
||
def self.transform_key(name) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. can we handle the case where There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. and looks like I have added check before method calling There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
case Panko.configuration.key_type | ||
when Panko::Configuration::CAMEL_CASE_LOWER then name.camelize(:lower) | ||
when Panko::Configuration::CAMEL_CASE then name.camelize | ||
else name | ||
end | ||
end | ||
|
||
def ==(other) | ||
return name.to_sym == other if other.is_a? Symbol | ||
return name == other.name && alias_name == other.alias_name if other.is_a? Panko::Attribute | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,23 @@ | ||
# frozen_string_literal: true | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I was talking about the documentation here: https://github.com/yosiat/panko_serializer/tree/master/docs |
||
|
||
module Panko | ||
class Configuration | ||
InitializeError = Class.new(StandardError) | ||
|
||
CAMEL_CASE_LOWER = "camelCase" | ||
CAMEL_CASE = "CamelCase" | ||
AVAILABLE_KEY_TYPES = [CAMEL_CASE_LOWER, CAMEL_CASE].freeze | ||
|
||
attr_reader :key_type | ||
|
||
def initialize | ||
@key_type = nil | ||
end | ||
|
||
def key_type=(value) | ||
raise InitializeError, "Invalid key type" unless AVAILABLE_KEY_TYPES.include?(key_type) | ||
|
||
@key_type = value | ||
end | ||
end | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree with you, so please:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added test for using configuration with alias