fix: no blank children names #666
Conversation
| field_name="", | ||
| # this is a fake field name, but it makes it possible to debug which type variant | ||
| # is the source of an exception downstream | ||
| field_name=f"{field_name}__{arg.__name__}", |
There was a problem hiding this comment.
is there a reason we couldn't
- just use
field_name - specify
arg.__name__as a separate field on field_meta
current approach here feels bad
There was a problem hiding this comment.
Currently, I believe the main use is mapping this back to the model itself. For subfields, guessing currently unused so field_name could work and feels preferable to above.
What is the issue with having no field name? Is this to improve exception message based on test? If that is the case then maybe could update on that side instead
There was a problem hiding this comment.
What is the issue with having no field name?
I ran into this when attempting to run a FactoryModel.build() command. The ParameterException said that a factory for a type was missing, but it the name was blank, so I couldn't determine which field was causing the error.
For subfields, guessing currently unused so
field_namecould work and feels preferable to above
Great, I'll change the PR to just use field_name instead.
There was a problem hiding this comment.
I ran into this when attempting to run a FactoryModel.build() command. The ParameterException said that a factory for a type was missing, but it the name was blank, so I couldn't determine which field was causing the error.
Where is that error coming from? Does the type matter there?
Probably clearer to have field_name regardless
There was a problem hiding this comment.
Where is that error coming from? Does the type matter there?
I think it does, since in the case of a union type it would be unclear what specific type is missing a factory.
| @@ -106,7 +106,7 @@ def is_new_type(annotation: Any) -> "TypeGuard[type[NewType]]": | |||
|
|
|||
|
|
|||
| def is_annotated(annotation: Any) -> bool: | |||
| """Determine whether a given annotation is 'typing.Annotated'. | |||
| """Determine whether a given annotation is 'typing.Annotated', or a similar typing annotation such as Optional. | |||
There was a problem hiding this comment.
Is this correct? The function looks for annotated origin or similar
There was a problem hiding this comment.
this is why i left this comment, it's pretty tricky
In [12]: a
Out[12]: typing.Optional[int]
In [13]: a.__args__
Out[13]: (int, NoneType)
In [14]: b = int | None
Out[14]: int | None
In [15]: b.__args__
Out[15]: (int, NoneType)
There was a problem hiding this comment.
Does this match _AnnotatedAlias or other conditions?
There was a problem hiding this comment.
I'm not familiar with _AnnotatedAlias, not sure...
There was a problem hiding this comment.
Based on testing in terminal think this handles this correctly so fine as is?
>>> from polyfactory.utils.predicates import is_annotated
>>> from typing import Optional
>>> is_annotated(Optional[int])
False
>>> is_annotated(int | None)
False
There was a problem hiding this comment.
Yeah, I don't think the functionality is broken here, it was just challenging to understand what was going on so I added the comment.
There was a problem hiding this comment.
I don't think this checks optional type based on https://github.com/litestar-org/polyfactory/pull/666/files#r2014961152. Can this be reverted?
without this, a user cannot debug which field is failing to generate
iloveitaly
force-pushed
the
children-names
branch
from
9d1a04b to
019fc61
Compare
iloveitaly
force-pushed
the
children-names
branch
from
019fc61 to
3f202c2
Compare
|
Documentation preview will be available shortly at https://litestar-org.github.io/polyfactory-docs-preview/666 |
|
@adhtruong updated this! Tests are passing, ready for final review. |
| @@ -634,6 +636,49 @@ class A(BaseModel): | |||
| assert AFactory.build() | |||
|
|
|||
|
|
|||
| @pytest.mark.skipif(sys.version_info < (3, 10), reason="requires modern union types") | |||
There was a problem hiding this comment.
Can these be split out so this is tested on all python versions?
| @@ -106,7 +106,7 @@ def is_new_type(annotation: Any) -> "TypeGuard[type[NewType]]": | |||
|
|
|||
|
|
|||
| def is_annotated(annotation: Any) -> bool: | |||
| """Determine whether a given annotation is 'typing.Annotated'. | |||
| """Determine whether a given annotation is 'typing.Annotated', or a similar typing annotation such as Optional. | |||
There was a problem hiding this comment.
I don't think this checks optional type based on https://github.com/litestar-org/polyfactory/pull/666/files#r2014961152. Can this be reverted?
Description
ensure no blank field names on children variants