-
-
Notifications
You must be signed in to change notification settings - Fork 364
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
Implement pyright support via dataclass_transforms #796
Conversation
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 for pyright
. The upstream spec specifies that the __dataclass_transform__
decorator is defined on a per-project basis either in the a .py
or .pyi
source file, essentially as a compatibility layer until this is moved into typing_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 a def
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.135 pyright
implementation if we:
- Define a
__dataclass_transform__
decorator in__init__.pyi
file. - Define a
__dataclass_transform__
method in__funcs.py
and import in__init__.py
.
Then a custom annotation of the form has types inferred properly.
import attr
@attr.__dataclass_transform__(fields=(attr.field, attr.attrib))
def custom_define(cls):
return attr.define(cls)
@custom_define
class Custom:
a: int
However, given that this is a very early and provisional specification and implementation in pyright
, adding a "surprisingly functional" shim in attrs
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__
.pyi
definition, 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 the import attr.__dataclass_transform__
behavior and instead wait until the spec bakes enough for the dataclass_transform
to show up in typing_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.
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. 🤞 |
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 |
Fully agree with this. My operating assumption is that this is highly 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? |
This is ready for a "near final" review. |
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 :)
Oh and some markup problems too – check out https://attrs--796.org.readthedocs.build/en/796/types.html#pyright pls. I looks like unbalanced backticks |
OK thank you very much for the quick turnaround everyone; I'm very 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? |
JFTR I've unpinned in f41db43 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. |
The code and documentation look good to me. |
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
|
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) |
Yes, Pylance has had this dataclass transform support since version 2021.4.2 (released a couple of weeks ago). |
Implements baseline support for
pyright
as 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 our contribution 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.
frozen
either in upstream spec or attrs docs.New features have been added to our Hypothesis testing strategy..pyi
).tests/typing_example.py
.docs/api.rst
by hand.@attr.s()
have to be added by hand too.versionadded
,versionchanged
, ordeprecated
directives. Find the appropriate next version in our__init__.py
file..rst
files is written using semantic newlines.changelog.d
.If you have any questions to any of the points above, just submit and ask! This checklist is here to help you, not to deter you from contributing!