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.
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.
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.
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.
🤔 I've done a little test of this. Under the 1.1.135 pyright 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.
import attr
@attr.__dataclass_transform__(fields=(attr.field, attr.attrib))
def custom_define(cls):
return attr.define(cls)
@custom_define
class Custom:
a: intHowever, 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__.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 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.
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.
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. |
hynek
left a comment
There was a problem hiding this comment.
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
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 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.
frozeneither in upstream spec or attrs docs.New features have been added to our Hypothesis 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 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!