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

Add CacheCrispies::Base#transform_keys method #55

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

wujibear
Copy link

@wujibear wujibear commented Dec 17, 2022

I was asking about being able to transform keys to something like camelcase and tried this out.
Unsure of your needs for contributions, but let me know what you think of the idea.

With this I can now do the following:

# app/serializers/rest/v1_serializer.rb
class Rest::V1Serializer < CacheCrispies::Base
  transform_keys lambda { |key| key.to_s.camelize(:lower) }
end

# app/serializers/rest/v1/some_serializer.rb
class Rest::V1::SomeSerializer < Rest::V1Serializer
  serialize :id, :created_at, :some_thing
end

Serialized data will look like:

{
  "id": "123",
  "createdAt": "date",
  "someThing": "here"
}

I then have all my serializers inherit from this one and now all my keys will be transformed to lower camelcase

See: #52

@wujibear
Copy link
Author

Actually, this only seems to work if I put the method in the serializer that is serializing.
Doesn't seem to get inherited for some reason, investigating that.

@wujibear
Copy link
Author

K. Fixed that, now it checks for parent definitions if no proc or existing instance var is supplied.
This lets you write a pattern you can inherit from, but any serializer can explicitly set its own transform method for its attributes

@adamcrown
Copy link
Member

Thanks for the PR. This seems reasonable. I know JS tends to lean towards snake-cased variables and having the JSON the same way can make things simpler with object deconstruction.

I would want to see some specs to cover this new functionality before I would merge it though.

@wujibear
Copy link
Author

Any other thoughts on method naming, or implementation though?

@wujibear
Copy link
Author

Added a spec and docs

@wujibear
Copy link
Author

wujibear commented Feb 1, 2023

@adamcrown got those specs for you

@wujibear
Copy link
Author

@adamcrown Any other thoughts?
Would you want to merge this?

Copy link
Member

@adamcrown adamcrown left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is looking really good. I do have a couple small questions though. And apologies for the wait in getting back on this. I should have more time to keep up with this gem now for multiple reasons. So I should be able to come back to this PR soon soon after these comments are addressed.

attrib,
from: from,
transform_key(attrib),
from: from || attrib,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is the || attrib needed here?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not great at seeing GH notifications, sorry.
I plan to check out your comments here and address in the next week or two.

I'll need to get back in the code here to recall correctly, but I believe this was meant as an improvement.
It seems like it calls for adding a comment to explain why, lol

```ruby
# app/serializers/base_serializer.rb
class BaseSerializer < CacheCrispies::Base
transform_keys lambda { |key| key.to_s.camelize(:lower) }
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we use a block here rather than a lambda argument?

It just reads a bit better and is a bit more consistent to me to have

transform_keys lambda { |key| key.to_s.camelize(:lower) }

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like you just repeated the line I have there... accident yes?
You're meaning to have something like this instead?

transform_keys do |key|
  key.to_s.camelize(:lower)
end

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.

2 participants