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

Add warning about the limitations of type inference when using Final #151

Merged
merged 8 commits into from
Apr 21, 2022

Conversation

sisp
Copy link
Contributor

@sisp sisp commented Jun 23, 2021

Unfortunately, I implemented type inference from a default value when using Final incorrectly in #150, i.e. it only works for simple cases, but it's actually not true type inference. I'm not sure whether it's currently possible to get the type hint inferred from the default value at runtime.

E.g.:

@dataclasses.dataclass
class A:
    data: Final = dataclasses.field(default_factory=lambda: [1, "a"])  # inferred as `list[int | str]`
  1. The implementation in Add support for Final type #150 does not support dataclasses.field(...), but this could be fixed.
  2. The implementation in Add support for Final type #150 would at best infer the default value to have the type list but cannot infer List[Union[int, str]]. In order to get the correct type, actual type inference is be needed.

This PR changes the behavior, so that a NotImplementedError is raised when Final has no argument and a default value exists. I don't think it's a big problem to not support this case because type inference is just a convenience. Instead, Final[...] could always be typed explicitly.

Sorry for the sloppiness in #150 ...

@sisp sisp force-pushed the fix-final-infers-from-default branch from 368d35e to 5d3e1c1 Compare June 23, 2021 09:54
@lovasoa
Copy link
Owner

lovasoa commented Jun 23, 2021

I think the behavior of #150 is ok... Of course you cannot get a complete type definition from just a single value, but using type can be useful in simple cases. Maybe just add a warning in this case ?

@sisp
Copy link
Contributor Author

sisp commented Jun 23, 2021

Good idea. I've pushed new commits. Something like this?

The commit message used for merging needs to be changed, perhaps you could use the commit message of 6393529.

@sisp sisp changed the title Revert support for type inference from default value when using Final Add warning about the limitations of type inference when using Final Jun 23, 2021
Comment on lines 589 to 591
"Support for type inference from a default value is limited and may "
"result in inaccurate validation. Provide a type to Final[...] to "
"ensure accurate validation."
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given this message, it will not be obvious where the issue comes from. I thing the warning should mention that it comes from marshmallow_dataclass, the name of the type, and the name of the field.

Copy link
Contributor Author

@sisp sisp Jun 23, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed. I've pushed a new commit, but I don't know how to get the name of the field. Is there a way to get it from within field_for_schema(...)?

@sisp
Copy link
Contributor Author

sisp commented Jun 23, 2021

I've added a warning about the limitations of using a default factory according to our discussion in #152.

Copy link
Owner

@lovasoa lovasoa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks and sorry for the delay !

@lovasoa lovasoa merged commit 660be17 into lovasoa:master Apr 21, 2022
@sisp sisp deleted the fix-final-infers-from-default branch April 22, 2022 07:22
@sisp
Copy link
Contributor Author

sisp commented Apr 22, 2022

No worries, thanks for merging the PR! 🙂

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.

2 participants