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 options to transform field keys non-recursively (#132) #310

Merged

Conversation

protestContest
Copy link
Contributor

@protestContest protestContest commented Feb 12, 2024

Following up on this discussion.

This adds two new values for the config option transform_fields:

  • camelize_shallow: Like camelize, but only affects the relationship and attribute keys, not their values.
  • dasherize_shallow: Like dasherize, but only affects the relationship and attribute keys, not their values.

Some users may have views with fields that are maps, and don't want the map's keys dasherized or camelized. The new config values allow these users to use the transform_fields option to transform only the field names, but not the values.

Feedback is requested. I don't really like the config value names—preferably we can think of something better. If this seems like the right direction, I'll continue to update the documentation.

@protestContest protestContest requested a review from a team as a code owner February 12, 2024 19:05
@protestContest protestContest marked this pull request as draft February 12, 2024 19:06
@mattpolzin
Copy link
Member

I'm not opposed to the current naming, but since you are interested in alternative ideas: "shallow" is an alright word for what you're doing here.

@mattpolzin
Copy link
Member

This seems like a good direction to go in. I'll do a more in-depth look at your implementation of expand_fields_once after you've wrapped up your first draft.

@protestContest protestContest force-pushed the transform-fields-nonrecursive branch 3 times, most recently from eb0593d to fae926b Compare February 12, 2024 23:58
@protestContest
Copy link
Contributor Author

Ok, I've updated the config values to :camelize_shallow and :dasherize_shallow and updated the docs around this config option. I also included a couple minor unrelated fixes to appease the linter.

@protestContest protestContest marked this pull request as ready for review February 13, 2024 00:02
@protestContest protestContest changed the title WIP: Add options to transform field keys non-recursively (#132) Add options to transform field keys non-recursively (#132) Feb 13, 2024
Copy link
Member

@mattpolzin mattpolzin left a comment

Choose a reason for hiding this comment

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

This feels a bit pedantic, but I noticed that expand_fields/2 handles all sorts of data types whereas expand_fields_once/2 only handles maps. On one hand, it only needs to in order to handle the job of transforming only the names of attributes of the map within JSON:API data. On the other hand, it's a utility function with theoretically broader usage that doesn't do the same thing as expand_fields/2 but only once as its name suggests.

Any other ideas what to call it? Off the cuff I'd say expand_map_fields_once/2 but I'm not tied to that name.

Aside from that small detail, this looks good!

@protestContest
Copy link
Contributor Author

Totally. What do you think of expand_keys? That sounds more descriptive to me.

@mattpolzin
Copy link
Member

expand_root_keys? I'm good with expand_keys too since it no longer sounds like it is as closely related to expand_fields, but expand_keys would need some code-documentation explaining that it only looks at keys in the root of the map (perhaps that documentation is useful regardless of the name).

@protestContest
Copy link
Contributor Author

expand_root_keys sounds good to me, I'll update that and add some docs.

@protestContest protestContest force-pushed the transform-fields-nonrecursive branch from fae926b to dcbf608 Compare February 15, 2024 23:03
Copy link
Member

@mattpolzin mattpolzin left a comment

Choose a reason for hiding this comment

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

This looks great. Thank you!

@mattpolzin mattpolzin merged commit 262b9cb into beam-community:main Feb 16, 2024
16 checks passed
@mattpolzin
Copy link
Member

I'll cut a new release shortly. #311

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