Support unannotated converters for attr.ib #12815
Conversation
This comment has been minimized.
This comment has been minimized.
Instead of only saving the converter's type, and infering the init argument type from that every time, directly save the init type, to support unannotated converters (like lambdas) more easily. For attributes that do not have a converter, have None for the Converter instead of Converter(None).
Infer the type of the respective argument to the __init__ function to be of type `Any`.
Make the type of the respective __init__ argument Any.
This comment has been minimized.
This comment has been minimized.
|
Should I keep resolving new conflicts? Is anybody going to review this PR? |
|
I'll take a look next week. Sorry for the wait! |
|
Great, thanks! No worries. I'll try to get to resolving the conflicts by then :) |
|
|
||
| @attr.s | ||
| class Bar: | ||
| field = attr.ib(default=None, converter=foo) |
There was a problem hiding this comment.
Could you add some tests to ensure the behavior is correct here?
For example, use reveal_type(i.field) on an instance of Bar. Construct an instance with Bar(1) or some other type.
There was a problem hiding this comment.
Good idea, thank you! Is this better now?
|
|
||
| @attr.s | ||
| class Bar: | ||
| name: str = attr.ib(converter=lambda s: s.lower()) |
| is_attr_converters_optional: bool = False, | ||
| is_invalid_converter: bool = False) -> None: | ||
| self.type = type | ||
| self.is_attr_converters_optional = is_attr_converters_optional |
There was a problem hiding this comment.
Why don't we need these attributes any more?
There was a problem hiding this comment.
Because the init_type inference logic was moved from Attribute initialization to parse_converter() and all of those attributes were only used for this inference. Now they are used internally on the parsing of the converter, but are not carried around in the Converter object, only the relevant init_type.
In a lot more detail:
Converter.type was holding the type of the function that was used as a converter. Now that we want to also deal with converters that do not have such a type, this field would be less meaningful. As this type is only used to infer an init_type for the attribute, and we now already have the init_type, we do not need to keep this value.
is_attr_converters_optional was used before to take the (deserialized) Converter object and when inferring an init_type from it and making it that type a Union with NoneType if optional. This is still done all the same, but directly on parsing the converter, so this internal value does not have to be saved and passed (and [de]serialized).
Same thing with is_invalid_converter: before it was kept in the Converter info so that if it is True AnyType(TypeOfAny.from_error) can be given as the init_type, now we directly keep the appropriate init_type.
There was a problem hiding this comment.
Thanks, that clears it up.
This comment has been minimized.
This comment has been minimized.
`reveal_type()` and check output in the test for unannotated attrs converter and lambda as a converter.
|
According to mypy_primer, this change has no effect on the checked open source code. 🤖🎉 |
### Description Fixes python#6172 If an unannotated converter function or a lambda expression is passed as a converter to `attr.ib()`, instead of giving an error, just take the type of the respective argument of the generated `__init__()` function to be `Any`, as suggested by @JelleZijlstra and @cgebbe. ## Test Plan Add two tests: one that tests the example from the issue of an unannotated function, and one that tests an example with a lambda expression as a converter. Co-authored-by: t4lz <t4lz.git@gmail.com> Co-authored-by: Tal Zwick <tal.zwick@astrazeneca.com> Co-authored-by: Jelle Zijlstra <jelle.zijlstra@gmail.com>
Description
Fixes #6172
If an unannotated converter function or a lambda expression is passed as a converter to
attr.ib(), instead of giving an error, just take the type of the respective argument of the generated__init__()function to beAny, as suggested by @JelleZijlstra and @cgebbe.Test Plan
Add two tests: one that tests the example from the issue of an unannotated function, and one that tests an example with a lambda expression as a converter.