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 params to set_id block #16

Merged
merged 3 commits into from
Oct 15, 2019
Merged

Add params to set_id block #16

merged 3 commits into from
Oct 15, 2019

Conversation

henvo
Copy link
Contributor

@henvo henvo commented Oct 14, 2019

Hey thanks for creating this repo and keeping fast_jsonapi alive


A previous pull request added a block to the ObjectSerializer.set_id class
method, which allows passing a block to the set_id method. Currently
this block takes only one argument record:

set_id do |record|
  "#{record.name.downcase}-#{record.id}"
end

This PR adds another argument params to the block:

set id do |record, params|
  params[:admin] ?  record.id : "#{record.name.downcase}-#{record.id}"
end

This customization can be useful in situation where we serve different
clients that may need different IDs. One nice side effect is also that
the set_id method has the same method signature as the attribute
method.

This PR was copied from Netflix/fast_jsonapi#380

Henning Vogt added 2 commits January 20, 2019 12:55
Pull request #331 added a block to the ObjectSerializer.set_id class
method, which allows passing a block to the set_id method. Currently
this block takes only one argument `record`:

```
set_id do |record|
  "#{record.name.downcase}-#{record.id}"
end
```

This PR adds another argument `params` to the block:

```
set id do |record, params|
  params[:admin] ?  record.id : "#{record.name.downcase}-#{record.id}"
end
```

This customization can be useful in situation where we serve different
clients that may need different IDs. One nice side effect is also that
the `set_id` method has the same method signature as the `attribute`
method.
@stas
Copy link
Collaborator

stas commented Oct 15, 2019

@henvo thank you for the PR. This seems like a breaking change. @jopotts @kpheasey what do you think, should we consider the merge for this release?

@kpheasey kpheasey self-requested a review October 15, 2019 12:35
@kpheasey
Copy link
Contributor

@stas This looks good to me. Can you explain why this is a breaking change.

The optional second params argument continues a convention and extends functionality w/o sacrificing performance. I actually had a similar use case a month ago and it would have been helpful. We got around it by setting a current user on the model and then referencing it from there, which is not ideal.

@stas
Copy link
Collaborator

stas commented Oct 15, 2019

@henvo @kpheasey apologies, I missed the part where the params is optional.

@kpheasey kpheasey changed the base branch from master to dev October 15, 2019 13:28
@kpheasey
Copy link
Contributor

@henvo I updated the PR to merge into the dev branch. Can you resolve the conflicts?

@henvo
Copy link
Contributor Author

henvo commented Oct 15, 2019

Hey all thanks for the feedback. I fixed the merge conflicts.

@kpheasey kpheasey merged commit 587fb2c into jsonapi-serializer:dev Oct 15, 2019
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.

4 participants