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

Enhancement: Optional __model__ class field #445

Closed
Mityuha opened this issue Dec 3, 2023 · 5 comments
Closed

Enhancement: Optional __model__ class field #445

Mityuha opened this issue Dec 3, 2023 · 5 comments
Labels
enhancement New feature or request

Comments

@Mityuha
Copy link
Contributor

Mityuha commented Dec 3, 2023

Summary

Hi there.
Thank you for this useful library.
I tried to build some factories and found declaration of __model__ field a little bit annoyed.
Indeed, we already declare the model type in Generic params. Perhaps, it's enough.

Basic Example

Now we should declare factories this way (from documentation):

@dataclass
class Person:
    name: str
    age: float
    height: float
    weight: float


class PersonFactory(DataclassFactory[Person]):  # << Person type
    __model__ = Person  # << Person type again 🤔 

My suggestion is getting rid of Base class type declaration by making __model__ field optional for backward compatibility and/or deprecating it. So that

@dataclass
class Person:
    name: str
    age: float
    height: float
    weight: float


class PersonFactory(DataclassFactory[Person]):  # << Person type only here
    ...

Drawbacks and Impact

No response

Unresolved questions

I skimmed the source code/documentation quickly and found classmethod create_factory. This method allows you to define a factory imperatively.
The question is -- is it worth contributing into library to give the opportunity for doing the same declaratively?
If so I'm ready to implement this feature.


Note

While we are open for sponsoring on GitHub Sponsors and
OpenCollective, we also utilize Polar.sh to engage in pledge-based sponsorship.

Check out all issues funded or available for funding on our Polar.sh Litestar dashboard

  • If you would like to see an issue prioritized, make a pledge towards it!
  • We receive the pledge once the issue is completed & verified
  • This, along with engagement in the community, helps us know which features are a priority to our users.
Fund with Polar
@Mityuha Mityuha added the enhancement New feature or request label Dec 3, 2023
@guacs
Copy link
Member

guacs commented Dec 4, 2023

@Mityuha yeah, a PR would definitely be welcome if there's a clean way to not have to specify the type twice. That being said, I'm curious as to how you're planning on getting rid of the need to specify __model__? The __model__ value is used in various places so we can't just remove it.

Also, I think the aim should be to do it the other way around. By that I mean that instead of attempting to remove the setting of __model__, it'd be nice if there was a way to not have to specify the type in the class definition. So something like the following:

class PersonFactory(DataclassFactory):
    __model__ = Person

@Mityuha
Copy link
Contributor Author

Mityuha commented Dec 4, 2023

Hello, @guacs . Thanks for response.
So, about your curiosity of how I'm planning to get rid of the need to specify __model__. Actually, I think it's possible to do by calling get_args(Generic[T]), which returns the information about type T. And it seems what we need. \

About the way to not specify the generic type: I could be wrong but it seems that the generic type is needed for further type hints. It's a little bit unclear for me how to achieve the same effect (generic class) without Generic[Person].

Again, I might be mistaken about type hints since I haven't dive deep into the code yet.

I think I'm going to try to implement my hypothesis in a clean way and, if I cope with it, I will make a PR.
If nobody minds.

@guacs
Copy link
Member

guacs commented Dec 5, 2023

About the way to not specify the generic type: I could be wrong but it seems that the generic type is needed for further type hints

You are right.

calling get_args(Generic[T])

Yupp this should work. So, I think the solution would be to parse and set the value for __model__ if a value for __model__ has not already been provided.

I will make a PR.
If nobody minds

A PR would definitely be appreciated :)

@Mityuha
Copy link
Contributor Author

Mityuha commented Dec 8, 2023

PR #452 is ready. Please come and check it out

@guacs
Copy link
Member

guacs commented Jan 19, 2024

Added in #452.

@guacs guacs closed this as completed Jan 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants