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

Deep transform_keys for inline associations #220

Closed
trevorturk opened this issue Jul 27, 2022 · 4 comments · Fixed by #232
Closed

Deep transform_keys for inline associations #220

trevorturk opened this issue Jul 27, 2022 · 4 comments · Fixed by #232
Labels
enhancement New feature or request

Comments

@trevorturk
Copy link
Collaborator

Is your feature request related to a problem? Please describe.

I'd like to have an option for transform_keys to work down through inline associations. Currently, you need to repeat transform_keys within each association block.

Additional context

Here's a code example based on the README which demonstrates the issue:

class User
  attr_reader :created_at
  attr_accessor :articles

  def initialize
    @created_at = Time.now
    @articles = []
  end
end

class Article
  attr_accessor :user_id

  def initialize(user_id)
    @user_id = user_id
  end
end

class AnotherUserResource
  include Alba::Resource

  transform_keys :lower_camel

  attributes :created_at

  many :articles do
    # transform_keys :lower_camel
    
    attributes :user_id
  end
end

user = User.new
article = Article.new(1)
user.articles << article

AnotherUserResource.new(user).to_h

Note the commented-out transform_keys macro, which, when un-commented, results in the desired output. Here's an example from the console showing the difference:

# articles.user_id, not transformed into lower_camel
{:createdAt=>2022-07-26 21:18:29.721693 -0500, :articles=>[{:user_id=>1}]}
# articles.userId, transformed if you repeat the `transform_keys` within the inline association
{:createdAt=>2022-07-26 21:18:29.721693 -0500, :articles=>[{:userId=>1}]}

Ideally, I'd like to avoid repeating transform_keys for inline associations, but things do work as expected currently, so this is only a nice-to-have. I'm not sure if it would be convenient in Alba's code to make this possible, but I thought I'd mention it.

Thank you!

@trevorturk trevorturk added the enhancement New feature or request label Jul 27, 2022
@okuramasafumi
Copy link
Owner

@trevorturk I came up with the following code:

class AnotherUserResource
  include Alba::Resource

  transform_keys :lower_camel, cascade: true # <- `cascade` option

  attributes :created_at

  many :articles do
    # transform_keys :lower_camel # This line is not required but works as if this line exists
    
    attributes :user_id
  end
end

Here, cascade: true option has an effect to enable the same key transformation in inline associations.
I also thought true is unclear about what it actually does, so maybe cascade: :inline is better so that in the future we can add cascade: :all to override existing key transformation settings.

@trevorturk
Copy link
Collaborator Author

That makes sense to me. I was thinking perhaps deep_transform_keys as an option, but I like your idea of cascade: :inline. Perhaps if we see transform_keys in an inline associations, we accept that as an override, so then cascade: true would be fine, knowing it could be overridden.

Thank you for your consideration to this!

@trevorturk
Copy link
Collaborator Author

Today I was wondering if it would be worth making cascade the default behavior, so we wouldn't need any options. Although that may be considered a breaking change. I was surprised it didn't cascade, so arguably we could call it a bug fix?

In any case, I don't mind whatever the solution is, and frankly the current functionality is fine, just some minor cruft when defining a resource.

Anyway, thanks again!

@okuramasafumi
Copy link
Owner

Making cascade default seems reasonable to me. Yes, it's a breaking change, so we need to offer a cascade: false option for old behavior.
Unfortunately it's hard to produce deprecation warnings in this case, so we need to mention it in the document (CHANGELOG.md).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants