Support Pyright detection of constructor kwargs in attrs-based data classes
#2945
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #2945 +/- ##
=======================================
Coverage 99.64% 99.64%
=======================================
Files 117 117
Lines 17564 17564
Branches 3166 3166
=======================================
Hits 17502 17502
Misses 43 43
Partials 19 19
|
Dataclass transforms that Pyright can recognize and honor for things like constructor kwargs were added in 21.1.0, which was subsequently yanked, leaving 21.2.0 as the minimum existing version containing them.
This affects only two classes:
1. CancelScope - the only class (I think) that is commonly instantiated with
kwargs by users.
2. CancelStatus - the only class that is actually instantiated with kwargs in
Trio's own code.
No other classes appear to fall into either of these categories, and are left
untouched.
Obviously, `frozen=True` arguments are now redundant, so removed. Further, `slots=True` is now default, so that's removed from calls while those that didn't specify are now explicitly `slots=False`.
Similarly to previous commit for attr.frozen, `slots=True` is now default, so that's removed from calls while those that didn't specify are now explicitly `slots=False`.
|
If you're replacing |
Happy to do that, as well, but that really does require increasing the constraint to |
|
Added two more commits for additional shorter syntaxes:
Along with a fix to a small typing error that this uncovered. The original line in _next_send_fn: Callable[[Any], object] = attr.ib(default=None)The type annotation didn't allow the default value. I updated the annotation. |
There was a problem hiding this comment.
Only skimmed the changes, but seems fine to merge. For ease of review it's perhaps not great to bundle a bugfix with a verbose style change, (and slots changes, with is coolcat's hobbyhorse :P) but probably not worth splitting it up at this point.
Another plus would be a regression test in src/trio/_tests/type_tests/, which are run with both pyright and mypy. The overkill approach would be something that comprehensively tested all parameters to functions, but given how often trio/_tests/test_exports is a hassle it might even be net negative.
Very happy to be moving to modern usage where reading the official docs for attrs is much easier, it confused me a bit when I first started familiarizing myself with the code.
| _cancel_status: CancelStatus | None = attr.ib(default=None, init=False) | ||
| _has_been_entered: bool = attr.ib(default=False, init=False) | ||
| _registered_deadline: float = attr.ib(default=inf, init=False) | ||
| _cancel_called: bool = attr.ib(default=False, init=False) | ||
| cancelled_caught: bool = attr.ib(default=False, init=False) | ||
| _cancel_status: CancelStatus | None = attrs.field(default=None, init=False) | ||
| _has_been_entered: bool = attrs.field(default=False, init=False) | ||
| _registered_deadline: float = attrs.field(default=inf, init=False) | ||
| _cancel_called: bool = attrs.field(default=False, init=False) | ||
| cancelled_caught: bool = attrs.field(default=False, init=False) | ||
|
|
||
| # Constructor arguments: | ||
| _deadline: float = attr.ib(default=inf, kw_only=True) | ||
| _shield: bool = attr.ib(default=False, kw_only=True) | ||
| _deadline: float = attrs.field(default=inf, kw_only=True, alias="deadline") | ||
| _shield: bool = attrs.field(default=False, kw_only=True, alias="shield") |
There was a problem hiding this comment.
if it was me I'd perhaps do from attrs import define, field, Factory etc, since we're only using a couple different objects from attrs, reuse them a bunch of times, and are widely used enough in the codebase I don't think there'd be much confusion about what library they're from. (and as opposed to attr.ib where ib on its own doesn't make sense). E.g. in this block it would cut down on the sheer number of characters a decent amount.
But seems plenty fine to stick with import attrs
|
Hey @mikenerone, it looks like that was the first time we merged one of your PRs! Thanks so much! 🎉 🎂 If you want to keep contributing, we'd love to have you. So, I just sent you an invitation to join the python-trio organization on Github! If you accept, then here's what will happen:
If you want to read more, here's the relevant section in our contributing guide. Alternatively, you're free to decline or ignore the invitation. You'll still be able to contribute as much or as little as you like, and I won't hassle you about joining again. But if you ever change your mind, just let us know and we'll send another invitation. We'd love to have you, but more importantly we want you to do whatever's best for you. If you have any questions, well... I am just a humble Python script, so I probably can't help. But please do post a comment here, or in our chat, or on our forum, whatever's easiest, and someone will help you out! |
@jakkdl By way of explanation, because the Pyright fix required a bump to the |
mikenerone
deleted the
mikenerone/fix-pyright-errors-private-attrs-construction
branch
[The changes related to this update fall into three categories:
attrs>=23.2.0is now required (based on this comment thread plus needing to bump to at least 22.2.0 anyway to support Pyright).CancelStatusandCancelScope(this is required for Pyright to recognize private attributes as constructor kwargs).attrsper previous chat discussion.import attr->import attrseverywhereattr.ib->attrs.fieldattr.s->attrs.defineorattrs.frozen, depending on existingfrozenkwarg and with adjustments made to all calls to account forslots=Truenow being the default).attrs.field(factory=X)->attrs.Factory(X)attrs.field(default=X)-> simply assign XNote: there's no new logic to be tested. All existing tests pass unchanged (except for non-logic name changes resulting from item 3 above).