-
-
Notifications
You must be signed in to change notification settings - Fork 2
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
ASYNC912: Warn on unnecessary checkpoints added to avoid ASYNC910/911 #70
Comments
Yeah 1 sounds great.
But biggest downside is probably not implementation time, but that the class is already 300 lines long and I do not envy anybody trying to understand it (imagine a bug turns up in A libCST fixer for #1 is pretty trivial, #2 sounds kinda nasty. Feels like for that case you write a script that parses the output of flake8 and |
OK then - let's ship (1), then have a big refactoring pr for clarity and perf with no semantic changes, and after that's in a final pr with the new unnecessary-manual-checkpoint check. LibCST doesn't seem worth the implementation and maintenance cost sadly, though it's possible that might change eventually. I agree that sed is a compelling alternative for checkpoint removal at least! |
Before tackling unnecessary checkpoints I wanted to finish up the reformat of visitors as per #83, to make This is now the second time in a short time I start thinking about implementing functionality (previous one being matchers to replace ugly/unreadable Also looking around, libcst has https://libcst.readthedocs.io/en/latest/visitors.html?highlight=leave#batched-visitors which is what I spent a bunch of time getting So I'm definitely starting to consider the idea of having I'm not 100% sure on all the pros/cons of switching or whether I ultimately think it's a good idea, but it would at the very least enable easily adding autofixing. |
Yeah, based on the possibility of autofixing, this actually seems pretty promising. After we get
|
Moved discussion of libcst to #124 |
Is this high prio enough you want it before libcst? I would prefer not to touch the mess that is 91x until afterwards, but I can prioritize converting 91x early for libcst (which is also a good stress test) and implement this soon after that's working. Tentatively marking it postponed |
Definitely postpone, I'd always try to use this via a janky autofix script, so doing it properly from the start sounds way better. |
I've noticed a few places where
await trio.sleep(0)
is added in places that don't actually need a checkpoint, and so I've thought up a two-part plan to mitigate cargo-cult copy-paste coding.await trio.sleep(0)
, suggestawait trio.lowlevel.checkpoint()
. It's an identical runtime effect, but the name is much more suggestive of what's actually going on: "we need a checkpoint here for some low-level reason" ✅await trio.lowlevel.checkpoint()
, check if there would be no 107/108 warnings emitted if that checkpoint was removed (in addition to any already-suggested-removals, of course).I initially marked this as an "idea" issue rather than for implementation because while I'm fairly confident (1) is a good plan, (2) seems a little harder to implement. It's also triggering my "this feels pretty tedious" detector, but building a libCST-based autofixer also seems like a little too much duplication of effort, right? Even if we could autofix 105, 107, 108, 112, and 113; and hook it in to
shed --refactor
... tempting but probably not worth it 😕Thoughts?
The text was updated successfully, but these errors were encountered: