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

Refactor: Marshal #15

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

Conversation

brandonc
Copy link
Collaborator

@brandonc brandonc commented Nov 10, 2023

I reorganized the very large visitModelNode function (the internal logic that builds a Node struct by recursively descending into a jsonapi-annotated struct model) into a couple of abstractions:

  1. modelFieldCursor encapsulates reflection data about each field in a model struct.
  2. modelVisitor uses cursor method Next() to move the cursor to each field. It retains config about sideloading relations as included Nodes and provides the top level Visit() interface. Most of the field-specific code was relocated as methods of the visitor, each receives the cursor as an argument.

@brandonc brandonc force-pushed the brandonc/polymorphic_relationships branch from 625119e to bb4d09f Compare November 15, 2023 17:56
@brandonc brandonc force-pushed the brandonc/refactor_marshal_visitor branch 2 times, most recently from e6c015e to 88c2c67 Compare November 15, 2023 18:03
Base automatically changed from brandonc/polymorphic_relationships to main November 15, 2023 20:48
@brandonc brandonc force-pushed the brandonc/refactor_marshal_visitor branch from 88c2c67 to 31766c8 Compare November 15, 2023 21:05
@brandonc
Copy link
Collaborator Author

I don't think this is ready because I just benchmarked it and it appears to be slower than the main branch. I'll keep working on it.

@brandonc brandonc marked this pull request as draft December 22, 2023 23:07
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.

1 participant