Give Pyright what it wants (alias attributes everywhere)
#3114
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #3114 +/- ##
==========================================
+ Coverage 99.58% 99.71% +0.12%
==========================================
Files 121 122 +1
Lines 18157 23605 +5448
Branches 3272 4022 +750
==========================================
+ Hits 18082 23538 +5456
+ Misses 52 48 -4
+ Partials 23 19 -4
|
|
this makes me want to write a linter rule, but also feels ridiculous (and idk which flake8 plugin it would fit) to write an attrs+pyright-specific rule. How slow/fast is the test? If it's anything but trivial I could rewrite it as an ast visitor. |
Yeah lint rule would make more sense but unfortunately a lint rule would miss context that's helpful (like whether a class is exported). It takes 1.75 seconds to run locally, though that's with the overhead of starting pytest. |
Maybe? Here's the output at the end of I don't think it's an appreciable slowdown. Maybe it's a good idea to put EDIT: just the second optimization there changed it to 0.64s locally, so I think it's fine if not marked |
|
There’s another approach we could use, but it’d might require starting an additional interpreter. Run a script which monkeypatches Unsure if that’d be faster though. We could avoid the new interpreter if the monkeypatch was done before any tests import trio, but that means it affects all tests. Probably not too much of an issue since metadata does nothing, and just wrapping the function should be fairly safe. |
It's not significantly faster: I think the main improvement would be that it's slightly more reliable. It sounds annoying to implement though. |
|
@TeamSpen210 do you have any implementation ideas for your idea? I was thinking about it just for the gain in reliability, but I think we would then get a flakey test because we delete and reimport trio to get all the warnings. I guess it's fine if we just patch attrs cause then the reimport will still be using the patched attrs? |
|
Pushed an implementation using monkeypatching here. Unfortunately a downside I realised is that the monkeypatching code has to be either outside the |
|
I do like the "monkeypatch, and skip that test if not monkeypatched" plan 🙂 |
|
@TeamSpen210 @A5rocks I'd really like to get this merged so I can use it; is there anything I can do to help? |
Nope! I've just been pushing off working on some trio stuff for a bit, I'll incorporate better testing strategy soon:tm: (probably just |
|
Would you like me to make the monkeypatch approach a competing PR, or PR to yours, or do you want to stick with the current approach? |
|
If you wouldn't mind, you should be able to commit to this PR. |
|
Committed, didn't want to do big changes to other people's PRs without permission. Though it is easy to revert so probably that's fine. |
|
Should we move the plugin to the (adjacent to |
|
That should work, if it's in |
Nevermind! I didn't realize pytest's sys.path schenanigans are the way they are. I'm surprised the test passes in CI! Maybe you can explain to me? I'm obviously missing something, because my mental model is:
It feels like this shouldn't work -- if this is a pytest bug, I literally see no other way of adding to the path. The |
|
Not really sure either, but it's working so don't touch it? |
|
Ohhhhh nooooo, it's installing As a knee-jerk reaction I think we should revert back to before using a plugin. Maybe you see a way to work around this? Probably setting list of packages in `site-packages`
|
|
We could set Another approach would be to use |
|
The plugin script can't be anywhere in the |
Yeah the issue with that commit was that we had nevermind, I should know better than make statements based on things running locally... turns out conftest always worked except for |
I don't like pyright nor this behavior, but that doesn't mean users should have to care. We can add a test to prevent any sort of regression regarding this, forever.
cc @mikenerone cause this is from Gitter.
also @CoolCat467 I ran into issues with running the
regenerate-filespre-commit hook and couldn't figure them out after about 15 minutes. It was complaining aboutattrsnot being possible to import but unfortunately there's no way to get it to use thetest-requirements.txtfile. Have you run into this issue regarding it not picking up the virtual environment? (I'm using a seperate git client so that kind of makes sense, but now's the first time I ran into this...)