-
Notifications
You must be signed in to change notification settings - Fork 64
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
Extract PrimaryKeyQueryable from Queryable #455
Extract PrimaryKeyQueryable from Queryable #455
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Smart! I like this approach
Looks like there are some conflicts. Ping me when you're ready and I'll take another look + merge |
0acd88a
to
761e475
Compare
761e475
to
bfd71ed
Compare
@paulcsmith This is ready to be reviewed again. I did tweak it so that |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like this new approach of having them implement just the methods they own and including both modules 👍
Just one thought about removing the abstract def
@matthewmcgarvey is there anything else you need to do on this? If not, I think it's ready to merge in. |
@jwoertink it's ready, thanks |
Related to #453
If we want to support database views in the future, one step in getting there is removing the requirement of the database table having a primary key.
When the time comes to allow not setting a primary key, the
BaseQueryTemplate
not includePrimaryKeyQueryable
which will remove any ability to query by the primary key for that model.