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

DerivePartialModel: Allow specification of Entity via a Path in addition to as an Ident #2111

Closed

Conversation

jreppnow
Copy link
Contributor

PR Info

  • Content: With the current implementation when deriving DerivePartialModel, one must specify the Entity whose columns should be used as an Ident - meaning that one must import it into the current scope

  • This is somewhat unwieldy, especially when dealing with generated code, where one would prefer to just use the containing module, not the contents themselves

  • This PR instead allows one to specify a Path, meaning that no direct use is required anymore

  • Ticket: Sorry, should have probably created a ticket first - does not exist atm, but can create one if necessary.

Bug Fixes

  • allow the Entity to be specified as a Path, not just an Ident, alleviating the need to import the Entity struct whenever you want to use the DerivePartialModel macro

Breaking Changes

  • The changes should be backwards-compatible/a pure extension, since Paths are a superset of Idents

Changes

  • fixed a typo in the changed file ("specific" to "specified") in a couple of places

@jreppnow jreppnow force-pushed the fix/reppnj/allow-path-for-partial-model branch 2 times, most recently from c116a1b to 4a4b11e Compare February 15, 2024 09:49
@jreppnow jreppnow force-pushed the fix/reppnj/allow-path-for-partial-model branch from 4a4b11e to 0c5e8e8 Compare February 15, 2024 09:50
Copy link
Member

@tyt2y3 tyt2y3 left a comment

Choose a reason for hiding this comment

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

Thank you for the contribution. Can you add some test cases showcasing the behaviour?

@tyt2y3
Copy link
Member

tyt2y3 commented Mar 10, 2024

I think the intent of this PR is mostly already covered by #2137
If you have something to amend, please rebase onto master, thanks!

@jreppnow
Copy link
Contributor Author

@tyt2y3 Yup, that one pretty much covers it! Sorry for taking a bit longer to respond, I will close this PR as it's been superseded.

@jreppnow jreppnow closed this Mar 11, 2024
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.

2 participants