Open asford opened 3 years ago
Yes, Eric was kind enough to reach out to me a few weeks ago and we discussed the topic.
Their integration doesn't cover all of our features but it seems good enough β at least good enough to not deliver false positives. NG APIs should work too.
I was hoping for feedback from @euresti (or any other typing gurus around here, really) before going to town on it. But maybe we can indeed squeeze it into 21.1 β at least provisionally.
Also pulling in @Tinche who seems to have already played with it!
Seems to work for me. The cost/benefit analysis seems in favor of adding this: it's very easy for us to do and IDE support is very useful, particularly if you're not quite ready to start using mypy.
I've also verified this on an internal codebase and found it to be generally functional.
Pleasantly, this allows us to implement attrs
excellent advice on wrapping the decorator in a reasonably straightforward fashion, however as you mention is is a subset of both attrs' current semantics and the mypy-attrs plugin's current semantics.
I do feel that there's some value in having attrs
begin to support this provisionally. It's a huge potential boost to VS Code users and could be a good way to drive more feedback on the pyright
spec on additional "non dataclass" semantics like converter
.
This is interesting. Similar to an idea I had for supporting this in https://github.com/python/mypy/issues/5406#issuecomment-409233479
I wonder if this could be used in mypy itself. Might be complicated with all the caching mypy does, but ... maybe?
How does it work with the overloads? Do you only annotate the real implementation? Can these be put in a .pyi file?
@euresti did you get Ericβs e-mail? He sent it to your GitHub address. That would be the best way to get direct answers. :)
How does it work with the overloads? Do you only annotate the real implementation? Can these be put in a .pyi file?
Quick link to a gist-playground as well for interested parties:
https://gist.github.com/asford/cd3c8ebbf245df6c3666eec0852c171d
If someone wants to give it a shot, it would be great to squeeze this into the overdue 21.1 release that will come out next week come hell or high water. Seems like enough of y'all have played with it so I'd be delighted if y'all could cooperate on getting it done. :)
I threw a very minimal PR together at #796 IFF you would like to fast-track support for this provisionally in 21.1.
@jakebailey just pointed out something that I had missed previously. It appears that attr.ib
and attr.field
use the parameter name factory
. Dataclasses (and the dataclass_transform spec) use the name default_factory
. That mismatch means that the following example (which is used in the attrs documentation) generates errors in pyright:
from typing import List
import attr
@attr.s(auto_attribs=True)
class SomeClass:
a_number: int = 42
list_of_numbers: List[int] = attr.field(factory=list)
I can think of two solutions here:
default_factory
rather than (or in addition to) factory
default_factory
and factory
I don't like option 2 because it would be an attrs-specific accommodation to the spec. What do you think about option 1? Can you think of any other options I'm missing?
factory=list
is syntactic sugar for default=Factory(list)
that has been added after much complaining over the years. Making it long again would almost completely obliterate the point of its very existence.
So in this case it would be really nice if we could go with option 2 (if making it configurable is out of the question). I know that my code bases are full of the "sweet" alternative and I'd hate to change it to the long one.
@erictraut is there anything we can do, to move the factory
thing forward? πΆ Still getting pinged about it and would like to have an answer β ideally a positive one. π₯Ί
Would you consider adding support for the parameter name default_factory
as a parameter to attr.field
for consistency with dataclass
? You could still support factory
for backward compatibility and make it an error if both values are provided. Short of that, I don't see anything that you can do here other than advocating for more attrs-specific accommodations in the dataclass_transform spec. I'm reluctant to add attrs-specific accommodations, but it's an option to consider if there's sufficient demand.
I've been capturing all of the limitations in the spec as we discover them. After we've received feedback for a few months, I plan to go back and do a rev on the spec and the reference implementation.
Supporting both in attrs is definitely an option, but there's a huge amount of code bases that use the shorter way (including mine).
The thing with "demand" is that I really don't want to send a mob your way. I saw it play out with the current pydantic/delayed type annotations kerfuffle and I'm still disgusted by how it played out. I guess the lack of power-mongering is a weakness of mine, but I would prefer a solution that's good for everyone.
What if attrs added default_factory
and you added factory
? π
As for your limitations: I think it's worth mentioning field_hook
s.
Keep in mind that the more attrs-specific accommodations that I add to the spec, the tougher it will be to standardize it in PEP form. I've already received a fair amount of pushback on the proposal in the typing-sig. If the goal is to standardize this (and I still think that's desirable), then I want to be cautious about adding more "warts" that will be difficult to justify to the typing-sig and python-sig reviewers.
On the other hand, if we decide that we're not going to try to standardize this, then it will remain a pyright-specific mechanism. In that event, we have more flexibility to extend it. However, it's also less likely that attrs users with large existing code bases (like you) will be affected because they presumably started with mypy and are unlikely to switch to another type checker.
If you're willing to add default_factory
, we can see if that satisfies the needs of those who use attrs and pylance/pyright for their type checking.
When you say field_hook
, are you referring to the field_transformer
parameter? Is that something that affects type checking?
A random user of both packages here: I'd prefer partial support released sooner to full support released later ππΏ
I found that using private attributes. (i.e attributes that start with a _) causes pyright to miss them in the constructor.
Example
@attr.s(auto_attribs=True)
class Class:
_priv: Int = None
A = Class(priv=1) # pyright errors saying No parameter named "priv"
B = Class(_priv=1) # no pyright errors. But is an actual error.
Private attributes are not a standard behavior in dataclass, and they're not supported in the "dataclass transform" mechanism. This is one of several known limitations. If you want to use attrs with pyright, you would need to avoid using the private attribute mechanism. Another option is to explicitly specify an alias using a field descriptor.
Thank you for the link. I didn't dig deep enough in the attrs documentation it seems. Can you point me to an example of using the alias in the field descriptor? I am not finding any examples.
As a follow-up for those tracking this issued, support for factory
was added to the dataclass transform spec in PEP681.
Pyright doesn't support the validator
decorator that attr.field
supports.
So I can't do this without getting type errors:
import attr
@attr.define
class Thing:
x: int = attr.field()
@x.validator # error: member "validator" is unknown
def _check_x(self, attribute, value):
pass
Are there any plans to add support for attrs converter
functionality on the horizon?
This is a random comment (well, it certainly relates to attrs
+ pyright
, and particularly to converter
which is why I found this issue) -- to me the most annoying thing I find as a newish user (to type annotations) is definitely not being able to properly separate between public API / user facing types and "internal types" -- and converter
is sort of a symptom of the above not being possible. I.e. I use converter
in the rare case where I want an internal type on an object to be different from the user-facing type, and where I don't want any user-facing promises at all about what the public type is, let alone want users to manually instantiate them. So ideally I want to be able to separately annotate the type of the generated __init__
parameter from the type of the instance attribute that gets populated by it. But I can imagine having terse syntax for such a thing is somewhere between "not existent yet", "doesn't exist even in theory" or "may not ever be useful enough to a wide audience" :).
In a similar vein to @EmreTech's issue, fields that have the default value generated by a method also have pyright treat it as an instance of "str" and not the attributes offered by field()
:
import attr
@attr.define
class Thing:
"""Thing."""
a_num: int = attr.field(default=3)
b_num: int = attr.field() # Pyright: Fields without default values cannot appear after fields with default values
@b_num.default # Pyright: Cannot access member "default" for type "int" Member "default" is unknown
def _b_default(self) -> int:
"""Defaults b_num as double of a_num."""
return self.a_num * 2
This is how documentation suggests to use the default decorator, with the example provided causing the same message to be reported for attrs 23.2.0 and pyright 1.1.351.
pyright
is adding support forattrs
classes in static type checking through a source-level extension mechanism called dataclass transforms. This will provide support the strict-subset ofattrs
class semantics implemented as standard-librarydataclasses
inpyright
, and by extensionpylance
and VS Code.Relevant project issues include:
https://github.com/microsoft/pyright/discussions/1782 - Discussion thread for feature https://github.com/microsoft/pylance-release/issues/226 -
pylance
tracking issue. https://github.com/microsoft/pyright/issues/146 -pyright
ur-issue. https://github.com/microsoft/pyright/pull/1773 - Initial PR https://github.com/microsoft/pyright/blob/master/specs/dataclass_transforms.md - Current spec in masterWould y'all be open to a PR implemented the recommendations in the linked specification? Perhaps there's a dialog to be had to ensure that the upcoming next-gen decorator semantics can be supported via this extension mechanism? (I don't have enough of the picture in mind to understand whether this is true.)
edit - Updating cross-refs I'd originally missed and rephrasing description. This connection had already been made!