Skip to content

Fix relationships with &:proc shorthands - #58

Merged
stas merged 1 commit into
jsonapi-serializer:masterfrom
attilahorvath:fix-relationships-with-proc-shorthands
Feb 20, 2020
Merged

Fix relationships with &:proc shorthands#58
stas merged 1 commit into
jsonapi-serializer:masterfrom
attilahorvath:fix-relationships-with-proc-shorthands

Conversation

@attilahorvath

@attilahorvath attilahorvath commented Feb 13, 2020 •
edited by stas
Loading

Copy link
Copy Markdown
Contributor

What is the current behavior?

If a define a relationship with a &:proc shorthand, such as:

class MovieSerializer
  include FastJsonapi::ObjectSerializer

  has_many :stars, &:actors
end

The serialization fails with ArgumentError: wrong number of arguments (given 1, expected 0).

However, the same thing would have worked as expected had I used a full do .. end block here.

What is the new behavior?

This change fixes this behavior by doing the same arity check as it's been done already for attributes in Scalar.

It would be nice to extract this bit of code somewhere instead of being duplicated here but I couldn't really find a nice place for that as they don't share a common superclass or anything like that. I can refactor this if anyone has a better idea.

Checklist

Please make sure the following requirements are complete:

  • Tests for the changes have been added (for bug fixes / features)
  • Docs have been reviewed and added / updated if needed (for bug fixes /
    features)
  • All automated checks pass (CI/CD)

@attilahorvath attilahorvath mentioned this pull request Feb 13, 2020
Merged
3 tasks
@stas

stas commented Feb 14, 2020

Copy link
Copy Markdown
Collaborator

@attilahorvath apologies for a bit of delay in reviewing this...

Before we merge this, I'd like us to review all the parts where we work with procs and use the new internal API method you suggested call_object_block(). Based on the #59 it's clear that all these can leverage the same API call.

Functionality aside, let's document this method properly and remove extra tests from #59 once we update it to use this API call. I'm ok if you want to merge this one first and update the #59 later. Let me know what you think...

Thanks in advance! 🙇‍♂️

@attilahorvath

attilahorvath commented Feb 14, 2020

Copy link
Copy Markdown
Contributor Author

Hi @stas, thank you for the review.

I think it makes sense to merge this one first and then I'll update #59 to use this method.

@stas

stas commented Feb 14, 2020

Copy link
Copy Markdown
Collaborator

Sounds good.

@attilahorvath may I ask you to provide the docstrings for the new method, consider using the YARD format.

I'll merge it as soon as you do that. Thanks!

@attilahorvath

attilahorvath commented Feb 17, 2020

Copy link
Copy Markdown
Contributor Author

Okay, I've given it some more thought and came up with the following.

I have a small helper method that works with either proc or lambda style code blocks and makes sure to only pass as many parameters as they can take, even if they have default parameters defined (passing more parameters than a lambda could take was the root of all these issues).

This method will be needed in multiple source files so I put it in a new helpers.rb file which can be useful later for similar methods that can be used project-wide.

If you're happy with this, I'll update all other usages of proc calls across the gem to use this method so they won't have similar issues in the future.

@stas

stas commented Feb 20, 2020

Copy link
Copy Markdown
Collaborator

@attilahorvath this looks good, let's move forward with this if you have time! 🙇‍♂️

@attilahorvath

attilahorvath commented Feb 20, 2020

Copy link
Copy Markdown
Contributor Author

@stas If you're okay with this, then I think we could merge this branch and then I'll update #59 to use this method.

Or should I push all the updates in this branch and close the other PR?

@stas
stas merged commit 08a20d0 into jsonapi-serializer:master Feb 20, 2020
@stas

stas commented Feb 20, 2020

Copy link
Copy Markdown
Collaborator

🙇‍♂️ @attilahorvath

@attilahorvath
attilahorvath deleted the fix-relationships-with-proc-shorthands branch February 20, 2020 17:55
@stas stas mentioned this pull request Feb 27, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@attilahorvath @stas