Replace public usages of `functools.wraps` #2796

A5rocks · 2023-09-09T05:57:35Z

I'm not entirely sure this works right but I can try figuring out how to configure pyright in a day or something unless someone else wants to check!

Unfortunately yet another pyright incompatibility -- while we could just replace usages around methods this is probably easier?

codecov · 2023-09-09T05:59:51Z

Codecov Report

Merging #2796 (93e7d80) into master (c30c76f) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Additional details and impacted files

@@           Coverage Diff           @@
##           master    #2796   +/-   ##
=======================================
  Coverage   99.13%   99.13%           
=======================================
  Files         115      115           
  Lines       17206    17207    +1     
  Branches     3082     3082           
=======================================
+ Hits        17057    17058    +1     
  Misses        104      104           
  Partials       45       45

Files	Coverage Δ
trio/_core/_run.py	`100.00% <100.00%> (ø)`
trio/_highlevel_open_tcp_stream.py	`97.64% <100.00%> (ø)`
trio/_path.py	`100.00% <100.00%> (ø)`
trio/_socket.py	`100.00% <100.00%> (ø)`
trio/_util.py	`100.00% <100.00%> (ø)`

A5rocks · 2023-09-09T06:14:45Z

Huh the typing impact is kinda shocking?

jakkdl · 2023-09-09T11:32:26Z

Huh the typing impact is kinda shocking?

whoa, great you took the initiative to do this! I didn't know @wraps was that broken in typeshed. I can look at the socket errors on monday or tuesday if you don't wanna try and delve into that thorny mess before then. Could maybe pull it into #2774 and handle it all there

jakkdl · 2023-09-13T12:34:08Z

Huh the typing impact is kinda shocking?

whoa, great you took the initiative to do this! I didn't know @wraps was that broken in typeshed. I can look at the socket errors on monday or tuesday if you don't wanna try and delve into that thorny mess before then. Could maybe pull it into #2774 and handle it all there

oh, so there was no problem and it was just a matter of your first stab at _wraps being incorrect?

A5rocks · 2023-09-13T15:24:02Z

Eh, it's more about what you think the intention of wraps is: my first stab pretended wrapped functions are the thing they wrap. My second stab didn't do so.

Hnasar · 2023-09-13T15:57:01Z

I didn't know @wraps was that broken in typeshed.

Yeah unfortunately the stubs break the wrapped function's signature. This comment explains the situation well: python/mypy#5107 (comment)

jakkdl · 2023-09-16T08:25:12Z

I didn't know @wraps was that broken in typeshed.

Yeah unfortunately the stubs break the wrapped function's signature. This comment explains the situation well: python/mypy#5107 (comment)

Eh, it's more about what you think the intention of wraps is: my first stab pretended wrapped functions are the thing they wrap. My second stab didn't do so.

thank you! It broke my brain a little bit, but I now I get the distinction.

jakkdl · 2023-09-16T08:40:17Z

no wait, I'm confused again. I think I'm getting different behavior (in pyright) if not renaming the import to _wraps. Could you try just doing a straight from functools import wraps and see if that makes a difference, and if not I'd love a repro of what exactly it is that breaks.

TeamSpen210 · 2023-09-16T09:19:40Z

Just checked, MyPy doesn't have a plugin for wraps, but it does cherry-pick an older implementation in its copy of typeshed, which just treats it as an identity function.

jakkdl · 2023-09-16T09:56:57Z

no wait, I'm confused again. I think I'm getting different behavior (in pyright) if not renaming the import to _wraps. Could you try just doing a straight from functools import wraps and see if that makes a difference, and if not I'd love a repro of what exactly it is that breaks.

oh derp, the repro is #2775 (comment)

But replacing _wraps with wraps does seem to fix it for me? Which implies that pyright has some workaround they're using.
It'd be great to add pyright to CI, though we might want to wait with it until we don't have to duplicate the blacklist between mypy & pyright config.

A5rocks · 2023-09-16T17:21:39Z

Just quick comment: I don't think it's worthwhile having both in CI as they have many intentional discrepancies. The only benefit we would have is that our public API would be guaranteed to work... which is probably something we can guarantee another way? (Just typecheck a subset of tests? Type tests that work in both mypy and pyright? Something like that)

jakkdl · 2023-09-17T07:47:57Z

Just quick comment: I don't think it's worthwhile having both in CI as they have many intentional discrepancies. The only benefit we would have is that our public API would be guaranteed to work... which is probably something we can guarantee another way? (Just typecheck a subset of tests? Type tests that work in both mypy and pyright? Something like that)

yeah agreed, we'd probably want to strip one of them down a lot. But I've been running both in flake8-trio without too many problems, though that project is definitely doing a lot fewer complex things type-wise - and started with pyright.

A5rocks · 2023-09-22T05:30:47Z

Trying out pyright locally, I cannot reproduce wraps working! Not sure what's different about my setup, just using VSCode, ctrl+click then editing the file (e.g. replacing our def _wraps with the import from functools broke things with Argument of type "list[bytes]" cannot be assigned to parameter "self" of type "_SocketType" in function "__call__", then renaming _wraps -> wraps then getting rid of the as didn't fix anything).

jakkdl · 2023-09-22T14:47:28Z

I don't know what I did last time, but I made sure to do everything properly now ... and you are indeed right! Sorry for the confusion

We should totally add a test for this though, and even if properly setting up pyright is a bigger task - we could for the moment create a dedicated test file with the contents of #2775 (comment), and for the moment run pyright only on that file in check.sh and/or pre-commit. Maybe a directory trio/_tests/typed_files/ or even more specifically trio/_tests/pyright_type_checking/.

A5rocks · 2023-09-26T06:45:21Z

Well, that's probably not the implementation you wanted but I went ahead and added a regression test @jakkdl

TeamSpen210 · 2023-09-26T07:05:48Z

With wraps, instead of duplicating the definition it would be better to just put that in _utils or something, then import.
For the type-testing framework, it might be a little easier to just add a type_tests/ folder or the like, then just run each checker over that whole folder in the regular CI.

jakkdl · 2023-09-26T07:31:13Z

Well, that's probably not the implementation you wanted but I went ahead and added a regression test @jakkdl

lol yeah, I'm not a fan of tests with code in strings. Teamspen210's idea is the same as mine.

A5rocks · 2023-09-30T05:14:53Z

Well hopefully that works, I should learn my lesson at this point about not using the web editor ever... It's just too tempting :(

A5rocks · 2023-09-30T05:38:54Z

trio/_core/_run.py

@@ -72,7 +72,7 @@
 DEADLINE_HEAP_MIN_PRUNE_THRESHOLD: Final = 1000

 # Passed as a sentinel
-_NO_SEND: Final = cast("Outcome[Any]", object())
+_NO_SEND: Final["Outcome[Any]"] = cast("Outcome[Any]", object())


This should probably be reported as an issue but gehhhhh

CoolCat467

I think this makes sense, looks good to me.

jakkdl

Other than small question about check.sh this looks great!

check.sh

A5rocks · 2023-10-03T01:51:38Z

I'm going to merge this. Pypy 3.9 nightly failing on Windows happens elsewhere and this way we can ask for more testing of the types!

I think any further changes can be done in a new PR.

Replace public usages of functools.wraps

4e62939

A5rocks added 2 commits September 9, 2023 02:05

Fix a couple CI errors

1ba1940

Don't blank out on how wraps actually works....

9e3bac2

A5rocks added 2 commits September 13, 2023 06:06

Don't try to pretend wrapped functions are what they wrap

9998355

Fix pyright type completeness

87e8348

jakkdl added the typing Adding static types to trio's interface label Sep 13, 2023

Finish off TODO

9c2a386

A5rocks added 3 commits September 26, 2023 15:41

Basic type test framework

b0d2725

Small cleanup

1dfe84a

Also make sure to pass type checking on the type tests!

b5fbc06

A5rocks requested review from TeamSpen210 and jakkdl September 26, 2023 06:45

A5rocks added 2 commits September 26, 2023 15:46

Remove some unnecessary imports; oops

7bcdeee

Fix a pytest skipif mistake

d31e545

A5rocks added 5 commits September 30, 2023 01:05

PR review changes

b2b4563

Forgot to check against pyright too

0bc5d02

Merge branch 'master' into use-update-wrapper

e0fc64b

Mark wraps as a re-export from _util (hopefully flake8 likes this)

6fa7c88

Fix CI errors :(

a462585

A5rocks added 2 commits September 30, 2023 14:29

Possibly last fixes

dee395e

Fix weird mypy errors

3d54ac5

A5rocks commented Sep 30, 2023

View reviewed changes

CoolCat467 approved these changes Oct 1, 2023

View reviewed changes

Merge branch 'master' into use-update-wrapper

8e0e067

jakkdl approved these changes Oct 2, 2023

View reviewed changes

check.sh Outdated Show resolved Hide resolved

Remove redundent mypy run

93e7d80

A5rocks merged commit 170642e into python-trio:master Oct 3, 2023
27 of 28 checks passed

A5rocks deleted the use-update-wrapper branch October 3, 2023 01:51

TeamSpen210 approved these changes Oct 3, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replace public usages of `functools.wraps` #2796

Replace public usages of `functools.wraps` #2796

A5rocks commented Sep 9, 2023 •

edited

Loading

codecov bot commented Sep 9, 2023 •

edited

Loading

A5rocks commented Sep 9, 2023

jakkdl commented Sep 9, 2023

jakkdl commented Sep 13, 2023

A5rocks commented Sep 13, 2023

Hnasar commented Sep 13, 2023 •

edited

Loading

jakkdl commented Sep 16, 2023 •

edited

Loading

jakkdl commented Sep 16, 2023 •

edited

Loading

TeamSpen210 commented Sep 16, 2023

jakkdl commented Sep 16, 2023 •

edited

Loading

A5rocks commented Sep 16, 2023

jakkdl commented Sep 17, 2023

A5rocks commented Sep 22, 2023

jakkdl commented Sep 22, 2023 •

edited

Loading

A5rocks commented Sep 26, 2023

TeamSpen210 commented Sep 26, 2023

jakkdl commented Sep 26, 2023

A5rocks commented Sep 30, 2023

A5rocks Sep 30, 2023

CoolCat467 left a comment

jakkdl left a comment •

edited

Loading

A5rocks commented Oct 3, 2023

Replace public usages of functools.wraps #2796

Replace public usages of functools.wraps #2796

Conversation

A5rocks commented Sep 9, 2023 • edited Loading

codecov bot commented Sep 9, 2023 • edited Loading

Codecov Report

A5rocks commented Sep 9, 2023

jakkdl commented Sep 9, 2023

jakkdl commented Sep 13, 2023

A5rocks commented Sep 13, 2023

Hnasar commented Sep 13, 2023 • edited Loading

jakkdl commented Sep 16, 2023 • edited Loading

jakkdl commented Sep 16, 2023 • edited Loading

TeamSpen210 commented Sep 16, 2023

jakkdl commented Sep 16, 2023 • edited Loading

A5rocks commented Sep 16, 2023

jakkdl commented Sep 17, 2023

A5rocks commented Sep 22, 2023

jakkdl commented Sep 22, 2023 • edited Loading

A5rocks commented Sep 26, 2023

TeamSpen210 commented Sep 26, 2023

jakkdl commented Sep 26, 2023

A5rocks commented Sep 30, 2023

A5rocks Sep 30, 2023

Choose a reason for hiding this comment

CoolCat467 left a comment

Choose a reason for hiding this comment

jakkdl left a comment • edited Loading

Choose a reason for hiding this comment

A5rocks commented Oct 3, 2023

Replace public usages of `functools.wraps` #2796

Replace public usages of `functools.wraps` #2796

A5rocks commented Sep 9, 2023 •

edited

Loading

codecov bot commented Sep 9, 2023 •

edited

Loading

Hnasar commented Sep 13, 2023 •

edited

Loading

jakkdl commented Sep 16, 2023 •

edited

Loading

jakkdl commented Sep 16, 2023 •

edited

Loading

jakkdl commented Sep 16, 2023 •

edited

Loading

jakkdl commented Sep 22, 2023 •

edited

Loading

jakkdl left a comment •

edited

Loading