Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork411
Implement pyright support via dataclass_transforms#796
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
Uh oh!
There was an error while loading.Please reload this page.
asford commentedApr 27, 2021
Please view as a starting point, I don't have super strong ownership over this feature and would happily cede the conn if this should be fast-tracked for 21.1. This feature can be implemented via annotations on custom decorator wrappers for projects that want to use There are a number of disparities with the existing
|
| # Static type inference support via __dataclass_transform__ implemented as per: | ||
| # https://github.com/microsoft/pyright/blob/1.1.135/specs/dataclass_transforms.md | ||
| # This annotation must be applied to all overloads of "define" and "attrs" | ||
| def__dataclass_transform__( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
There should probably be an implementation of this somewhere right? Otherwise this won't be useful outside of attrs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
This is a little strange, the decorator is just being used as a marker forpyright. The upstream spec specifies that the__dataclass_transform__ decorator is defined on a per-project basiseither in the a.py or.pyi source file, essentially as a compatibility layer until this is moved intotyping_extensions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Hmm. I'm a little concerned (ok not too much but it definitely feels weird) about having adef in the pyi file that doesn't exist in the .py file. It means that someone could try to use this function in their own code but mypy wouldn't warn them that it doesn't really exist. It would fail at runtime. Are we sure we don't want to put an implementation in _funcs.py or something?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
🤔 I've done a little test of this. Under the 1.1.135pyright implementation if we:
- Define a
__dataclass_transform__decorator in__init__.pyifile. - Define a
__dataclass_transform__method in__funcs.pyand import in__init__.py.
Then a custom annotation of the form has types inferred properly.
importattr@attr.__dataclass_transform__(fields=(attr.field,attr.attrib))defcustom_define(cls):returnattr.define(cls)@custom_defineclassCustom:a:int
However, given that this is a very early and provisional specification and implementation inpyright, adding a "surprisingly functional" shim inattrs introduces a potentially risky coupling. We've a choice between:
- An immediate and loud runtime failure if you "try to use" the attrs-internal
__dataclass_transform__.pyidefinition, that won't be caught static type checkers. - A surprisingly functional form that "works" (a no-op a runtime, currently works at typingtime), but relies on unspecified upstream behavior.
Given how bleeding-edge this feature is I have a weakly held preference that we opt for 2 for this release to ensure that folks don't implictly rely on theimport attr.__dataclass_transform__ behavior and instead wait until the spec bakes enough for thedataclass_transform to show up intyping_extensions.
Of course, happy to reconsider.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Since this is only for pyright and doesn't currently get mypy to do the right thing (i.e. it doesn't tell mypy that this is an attrs class creator) perhaps it's best if we leave this only inside the pyi file. I might add a comment on here that says that this method doesn't actually exist at runtime.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Roger, comment added.
Uh oh!
There was an error while loading.Please reload this page.
hynek commentedApr 28, 2021
Ah@erictraut it would be great if you could give it a casual glance! If everything goes well, we'll be able to publish 21.1 within a week. 🤞 |
erictraut commentedApr 28, 2021
This looks good to me. My only word of caution is that my spec is not yet final. It's still in the review stage, so I'm a little nervous about a major library adopting it at this phase. On the other hand, the worst that would happen if I need to change the spec is that pyright/pylance users would be no worse off than they are today with respect to type support for |
asford commentedApr 29, 2021
Fully agree with this. My operating assumption is that this ishighly provisional and primarily intended to allow for rapid feedback on the upstream specification. @hynek, would you be comfortable with additional minor releases in this release series if/when the upstream spec evolves? |
asford commentedApr 29, 2021
This is ready for a "near final" review. |
hynek left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
lint is failing 😇
also could you please move the pyright stuff to 3.9? 3.8 is overflowing already anyways :)
hynek commentedApr 30, 2021
Oh and some markup problems too – check outhttps://attrs--796.org.readthedocs.build/en/796/types.html#pyright pls. I looks like unbalanced backticks |
hynek commentedMay 5, 2021 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
OK thank you very much for the quick turnaround everyone; I'mvery excited by this! I'm merging it but there's one thing that's a bit weird to me and that's the hard pin of pyright's version everywhere – down to the micro. I don't think this is necessary? |
hynek commentedMay 5, 2021
JFTR I've unpinned inf41db43 and added some docs polish. I didn't want to pester you with more review rounds, feel free to yell at me if you disagree with anything. I hope to publish later this week – as far as I can tell, everything has landed. |
erictraut commentedMay 5, 2021
The code and documentation look good to me. |
asford commentedMay 5, 2021 via email
Agreed, thank you for the polish on the docs. On Wed, May 5, 2021 at 07:46 Eric Traut ***@***.***> wrote: The code and documentation look good to me. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#796 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AACFBKHI34DIGY5AZPIGXBTTMFK6FANCNFSM43VVKLKA> . -- Alex Ford***@***.***206.659.6559 |
hynek commentedMay 5, 2021
Thanks all! Eric quick question since I don't have a good way to test this: does this mean that attrs 21.1 will also automagically work with pylance / VS Code? (Working on the release announcement) |
jakebailey commentedMay 5, 2021
Yes, Pylance has had this dataclass transform support since version 2021.4.2 (released a couple of weeks ago). |
Uh oh!
There was an error while loading.Please reload this page.
Implements baseline support for
pyrightas described in#795.Pull Request Check List
This is just a friendly reminder about the most common mistakes. Please make sure that you tick all boxes. But please read ourcontribution guide at least once, it will save you unnecessary review cycles!
If an item doesn't apply to your pull request,check it anyway to make it apparent that there's nothing left to do. If your pull request is a documentation fix or a trivial typo, feel free to delete the whole thing.
frozeneither in upstream spec or attrs docs.New features have been added to ourHypothesis testing strategy..pyi).tests/typing_example.py.docs/api.rstby hand.@attr.s()have to be added by hand too.versionadded,versionchanged, ordeprecateddirectives. Find the appropriate next version in our__init__.pyfile..rstfiles is written usingsemantic newlines.changelog.d.If you haveany questions toany of the points above, justsubmit and ask! This checklist is here tohelp you, not to deter you from contributing!