Closed ringohoffman closed 10 months ago
Hi @ringohoffman! If you can provide additional information on how this change interacts with:
This would significantly speed up the review process here, otherwise I'll have to try all these myself before I can review. Community had issues with this codepiece some time ago, so I have to be extra careful here. Also a sidenote, @overload
functionality is not a very stable thing to rely on for such change, but let's put that aside and get basic checks done first.
What I had already posted were the changes you can see in VS Code using pylance/pyright. Pylance is a Visual Studio Code extension that uses pyright at its core for static type checking. I've gone ahead and added the before and after for PyCharm and mypy to the description as well.
adding a simple test step that runs pylint/pyright would be fine
What do you mean by this? Do you want me to add a GitHub Workflow step to run pyright and pylint?
When I run pyright on the dataclasses_json
module, there are 25 pre-existing errors. This PR doesn't add to or resolve these errors. I don't think that adding a workflow step for this is feasible at this time. Moreover, I think the poor type-checking compatibility of the dataclass_json
decorator would completely preclude us from enforcing no pyright errors on this codebase.
I don't see any typecheck
warnings/errors when I run pylint on the code in my description, before or after. This PR does resolve a pylint warning for the unused LetterCase
import in dataclasses_json/api.py since we are using it in the type hints now. Given that pylint didn't catch anything related to these changes, I see adding pylint to the workflow for this repository as a separate issue, but one that I would support.
Let me know how you want to move forward based on this and my updated description.
I think it's ok, however I'm curious if this works with IDE autocomplete? You snippets are runtime code, but autocomplete does not rely on reveal_type etc. Though people mainly complain about static type checkers, we had cases when they also want IDE autocomplete to work like it does when using a Mixin.
Other small question, we plan to deprecate this function in v1 API (coming soon). How does that affect you?
I think it's ok, however I'm curious if this works with IDE autocomplete? You snippets are runtime code, but autocomplete does not rely on reveal_type etc. Though people mainly complain about static type checkers, we had cases when they also want IDE autocomplete to work like it does when using a Mixin.
Other small question, we plan to deprecate this function in v1 API (coming soon). How does that affect you?
I don't think it's possible to make dataclass_json
compatible with auto-complete. I think you have to choose the return type as either the wrapped type or as DataClassJsonMixin
, but either way static type checkers will lose access to part of the actually available runtime methods. I asked the author of pyright how he would handle making this function static-typing friendly, and he had recommended to just use the mixin as a mixin instead of the decorator.
This doesn't impact me in any major way. I already migrated flytekit
to use DataClassJsonMixin
as a mixin instead of the dataclass_json
decorator: https://github.com/flyteorg/flytekit/pull/1801 They only use dataclass_json
in that one place now: https://github.com/flyteorg/flytekit/blob/0a772f3621ce9bd1a3e3b2f1e691458f25a31876/flytekit/core/type_engine.py#L1549
This doesn't impact me in any major way. I already migrated flytekit to use DataClassJsonMixin
Good, though v1 deprecates both the Mixin and the annotation - check discussion in #442. Note that we'll release v1 alongside v0 and deprecation of v0 will take very long time so please do not be scared :) But it helps a lot for us to understand the use cases like the code snippet you referenced.
Thanks a lot for contribution, we have a bit of an issue with 3.7 rn, but I plan to go around it and release your commit in 0.6 in a few days.
Use
typing.overload
to differentiate the return type ofdataclass_json
based on whether_cls
parameter isNone
or not.This is motivated by fixing type issues in flytekit, which does not only use
dataclass_json
as a decorator.