feat: Optional __model__ type

[Mityuha]

Pull Request Checklist

• New code has 100% test coverage
• (If applicable) The prose documentation has been updated to reflect the changes introduced by this PR
• (If applicable) The reference documentation has been updated to reflect the changes introduced by this PR
• Pre-Commit Checks were ran and passed
• Tests were ran and passed

Description

For now you have to specify model type twice: both for a generic type and explicitly for a __model__ field:

@dataclass
class Person:
    name: str

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

This PR adds support for omitting __model__ field specification:

@dataclass
class Person:
    name: str

class PersonFactory(DataclassFactory[Person]):
    ... 

After PR is merged, next PR will be documentation fixes related.

Close Issue(s)

• Enhancement: Optional __model__ class field #445

[guacs]

This looks great! Just a few points regarding the tests and the documentation.

1. I think it'd be good to document this behavior as part of this PR itself.
2. In the tests, I don't think it's necessary to test that the build method works correctly. Instead, a check to see that the __model__ has been set properly is what should be tested since that's the behavior we're concerned about in these tests.
3. I think the tests can all be moved into one file, and the test for the happy case (i.e. the model is correctly available) can be tested by parameterizing. Something similar to what you've done in test_model_inference_error.py.

[Mityuha]

This looks great! Just a few points regarding the tests and the documentation.

1. I think it'd be good to document this behavior as part of this PR itself.
2. In the tests, I don't think it's necessary to test that the build method works correctly. Instead, a check to see that the __model__ has been set properly is what should be tested since that's the behavior we're concerned about in these tests.
3. I think the tests can all be moved into one file, and the test for the happy case (i.e. the model is correctly available) can be tested by parameterizing. Something similar to what you've done in test_model_inference_error.py.

Thank you for review. All changes within change request were done.

[guacs]

@Mityuha sorry about this, but I wasn't clear in my previous review regarding the documentation. I think we would need to explicitly document this behavior somewhere. That is state explicitly the following:

• users may skip providing the __model__ IF its type hinted in the generics
• users can provide just the __model__ and not provide the type in the generics
• if a __model__ is provided, then that will be used over whatever the value is given as the generic model type

[Mityuha]

@Mityuha sorry about this, but I wasn't clear in my previous review regarding the documentation. I think we would need to explicitly document this behavior somewhere. That is state explicitly the following:

• users may skip providing the __model__ IF its type hinted in the generics
• users can provide just the __model__ and not provide the type in the generics
• if a __model__ is provided, then that will be used over whatever the value is given as the generic model type

@guacs OK, not a problem. Where you think is it better to document such behaviour? I would suggest documenting it

• in the Usage Guide/Declaring Factories section. Maybe inside like a info block
• optionally in README. Maybe in the Example section with the reference to Declaring Factories section
• optionally in the main documentation page with the reference like in README

Why do I suggest noting this behaviour inside README/index? It's a proven fact that really small number of users read something besides README/index page. So any info block in main pages could maybe attract user's attention.
(Or maybe contrariwise we want to hide some implementation details by burying this behaviour in the depths of documentation).

What do you think?

[guacs]

@Mityuha sorry about this, but I wasn't clear in my previous review regarding the documentation. I think we would need to explicitly document this behavior somewhere. That is state explicitly the following:

• users may skip providing the __model__ IF its type hinted in the generics
• users can provide just the __model__ and not provide the type in the generics
• if a __model__ is provided, then that will be used over whatever the value is given as the generic model type

@guacs OK, not a problem. Where you think is it better to document such behaviour? I would suggest documenting it

• in the Usage Guide/Declaring Factories section. Maybe inside like a info block
• optionally in README. Maybe in the Example section with the reference to Declaring Factories section
• optionally in the main documentation page with the reference like in README

Why do I suggest noting this behaviour inside README/index? It's a proven fact that really small number of users read something besides README/index page. So any info block in main pages could maybe attract user's attention. (Or maybe contrariwise we want to hide some implementation details by burying this behaviour in the depths of documentation).

What do you think?

This is a good question. Here's what I'm thinking.

• Keep the example in the README.md and the README/index as it is now (i.e. with the explicit definition of __model__ as well as the model being specified as the generic argument. The reason I'm thinking of doing it this way is because I don't think documenting all these other scenarios in the first page itself feels right.
• Document the behavior in Declaring Factories in Usage section.

Thoughts, @Mityuha? I'd like your opinion as well, @JacobCoffee.

[Mityuha]

Thoughts, @Mityuha? I'd like your opinion as well, @JacobCoffee.

What I do think is that maybe we want to make life easier by omitting explicit __model__ definition (that's what my issue is about). From this point of view it would IMHO be better to place example without explicit __model__ definition at least for some reasons:

1. New library users will start using the new simple syntax without any problem. So this syntax raises fewer questions making the library more attractive. (Also documenting this new syntax in README/Index pages encourages using it. And it appears to be what we want to achieve).
2. The new syntax does NOT affect users who already use the library. By the way, if user comes back to documentation, there is a chance that the new syntax will be noticed. And here is pp.3
3. Also, I don't think that anyone will be happy with the transition from implicit __model__ syntax to explicit __model__. But the opposite is true.
4. Placing the new syntax in the depths of documentation makes these changes (new syntax) pretty useless IMHO. It will be like a sacred knowledge
5. IMHO the implicit __model__ syntax should have been considered as a target syntax because of its simplicity

I don't think documenting all these other scenarios in the first page itself feels right

Me neither. I didn't offer to document all scenarios in the first page. But! I think the appropriate link to all possible scenarios might be useful. But I'm not sure

[guacs]

Also documenting this new syntax in README/Index pages encourages using it

@Mityuha you're right. I think this is the better option. So, I'm thinking this is how we can do it:

• keep the example with the new syntax on the readme etc
• in the Declaring Factories section, document the whole thing

I think the appropriate link to all possible scenarios might be useful

I'm not sure if the link to the docs regarding the __model__ inference etc, is needed in the README itself. The README is just meant for a very quick introduction to the library.

[Mityuha]

Also documenting this new syntax in README/Index pages encourages using it

@Mityuha you're right. I think this is the better option. So, I'm thinking this is how we can do it:

• keep the example with the new syntax on the readme etc
• in the Declaring Factories section, document the whole thing

I think the appropriate link to all possible scenarios might be useful

I'm not sure if the link to the docs regarding the __model__ inference etc, is needed in the README itself. The README is just meant for a very quick introduction to the library.

@guacs @JacobCoffee Guys, please, check out the new note block in the Declaring Factories section.
Eventually, it turned out quite the way I wanted. I mean regarding explanation structure. But honestly speaking I thought that sphinx tables would look more attractive =)
Anyway, waiting for your comments.

[guacs]

Tbh, I didn't like the table thing all that too much. It feels like it makes it a bit more complicated than it needs to be. I think it might be simpler to just state that there are two ways of specifying the model type, along with examples of both. Then in a note section, mention that the new syntax is only available from version 2.13.0 onwards.

[Mityuha]

@guacs It appears that I misunderstood how to deal with it, because you said then

in the Declaring Factories section, document the whole thing

and now

to just state that there are two ways of specifying the model type

So I thought by document the whole thing you meant the complete explanation.

But it's OK. I fixed the docs page.
Actually I think that if there is something wrong again it would be more convenient for all if you'd just make small changes to make docs look as you see it.

[guacs]

@Mityuha thanks for being patient with me! Everything looks great, and this is a really nice feature that makes things a bit easier for users :)

[github-actions]

Documentation preview will be available shortly at https://litestar-org.github.io/polyfactory-docs-preview/452

[Mityuha]

@Mityuha thanks for being patient with me! Everything looks great, and this is a really nice feature that makes things a bit easier for users :)

@guacs Thank you as well for your review and comments. If I make up my mind -- and if you don't mind -- I will contribute for something else. I have one more idea by the way =)

[guacs]

If I make up my mind -- and if you don't mind -- I will contribute for something else. I have one more idea by the way =)

Feel free to do so! Also, I'm curious to know what the idea is :)