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

[Concept] Drop reduntant model param #120

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

Conversation

JakNowy
Copy link
Contributor

@JakNowy JakNowy commented Jul 4, 2024

The idea is that if you use EndpointCreator, the model is passed twice: once to the corresponding FastCRUD and another time class itself. As the model is already required in FastCRUD, it should picked from there.

This is a breaking change if somebody didn't pass explicit crud, but EndpointCreator(crud=FastCRUD(MyModel)) or EndpointCreator(crud=MyFastCRUD(MyModel)) looks a clean and consistent approach.

What are your thoughts? @igorbenav

@JakNowy JakNowy marked this pull request as draft July 4, 2024 13:25
@igorbenav
Copy link
Owner

Since passing a CRUD class is optional, I don't think this is relevant

@JakNowy
Copy link
Contributor Author

JakNowy commented Jul 8, 2024

Since passing a CRUD class is optional, I don't think this is relevant

That's a fair point, from perspective of somebody who uses generic crud endpoint. But making a CRUD required parameter of the endpoint would unify the approach in case you want to extend the router logic.

@igorbenav
Copy link
Owner

Now that I thought more about it, I think this is good. Specially because we could add a check that the crud's model is the same as the passed model if both are passed

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