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

feat: Use the association options to lookup relationship class #403

Conversation

sebasjimenez10
Copy link
Contributor

Hey there,

I'm opening this PR because I noticed an issue with the way the association class types are being looked up when including related resources.

I discovered this when having cross-referenced types when using namespaces, like the example suggests:

module CrossNamespaceTwo
  class Nail < TestResource
    property :size
  end
end

module CrossNamespaceOne
  class Hammer < TestResource
    property :brand

    has_many :nails, class: CrossNamespaceTwo::Nail
  end
end

In this case whenever the IncludedData class was initializing an instance, the utils class would not be able to produce a correct type to use for the related included type producing an error similar to the following:

Finished in 0.447569s, 538.4645 runs/s, 1619.8620 assertions/s.

  1) Error:
AssociationTest#test_cross_namespace_resource_references:
NameError: uninitialized constant CrossNamespaceOne::Hammer::Nail

Since one should be able to add options to the association definition the class option makes sense here. This PR makes use of that to try and figure out the appropriate type before defaulting to the existing behavior.

Thanks in advance for taking a look!

@sebasjimenez10 sebasjimenez10 changed the title Use the associations to lookup relationship class [Feature] Use the associations to lookup relationship class Aug 11, 2022
@sebasjimenez10 sebasjimenez10 changed the title [Feature] Use the associations to lookup relationship class [Feature] Use the association options to lookup relationship class Aug 11, 2022
@sebasjimenez10
Copy link
Contributor Author

Hey @gaorlov 👋 hope you can take a look at this one as well 🙂 thanks in advance!

@gaorlov
Copy link
Collaborator

gaorlov commented Aug 12, 2022

@sebasjimenez10 thanks for the contribution!
This looks very similar to the built in class_name functionality?

module CrossNamespaceOne
  class Hammer < TestResource
    property :brand

    has_many :nails, class_name: 'CrossNamespaceTwo::Nail'
  end
end

would that work for your usecase?

@sebasjimenez10
Copy link
Contributor Author

sebasjimenez10 commented Aug 13, 2022

@gaorlov thanks for the response! I went ahead and commented the changes I made to utils.rb and used class_name but I still got the:

NameError: uninitialized constant CrossNamespaceOne::Hammer::Nail

error.

Any ideas on why? I'm happy to try something else! 🙏

@sebasjimenez10
Copy link
Contributor Author

@gaorlov small bump on this one as well! 🙏

Copy link
Collaborator

@gaorlov gaorlov left a comment

Choose a reason for hiding this comment

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

Looks great! thanks for contributing!
Please merge in main to get tests running in the PR and please add this PR to the CHANGELOG.md
Thanks!

@sebasjimenez10 sebasjimenez10 force-pushed the inspect-associations-for-computing-type branch 2 times, most recently from 99d1399 to 036e84b Compare June 6, 2023 04:34
@sebasjimenez10
Copy link
Contributor Author

@gaorlov thanks for the reply! Merged in main into the branch and added the changelog updates!

@sebasjimenez10 sebasjimenez10 force-pushed the inspect-associations-for-computing-type branch from 036e84b to 50b05ab Compare June 6, 2023 04:53
@sebasjimenez10 sebasjimenez10 changed the title [Feature] Use the association options to lookup relationship class feat: Use the association options to lookup relationship class Jun 6, 2023
@sebasjimenez10
Copy link
Contributor Author

@gaorlov small bump! 🙏

@gaorlov
Copy link
Collaborator

gaorlov commented Sep 9, 2023

@sebasjimenez10 I'm on a trip right now, but i will merge and release a new version when I'm back next week. Thank you for your contributions and patience!

@sebasjimenez10
Copy link
Contributor Author

@gaorlov small bump! we're almost there! 🙏

@gaorlov gaorlov merged commit 17f1399 into JsonApiClient:master Nov 22, 2023
16 checks passed
@gaorlov
Copy link
Collaborator

gaorlov commented Nov 22, 2023

hello!
Sorry about the delay! this is now available in 1.22.0
Thank you so much for your contribution!

@sebasjimenez10 sebasjimenez10 deleted the inspect-associations-for-computing-type branch May 6, 2024 16:55
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.

None yet

2 participants