Localize cleanup for FunctionDef and ClassDef #10837
Conversation
|
Do you mind adding the "Internal" label to this one? We have our release infrastructure configured such that PRs labeled "Internal" are automatically filtered out from the release notes, so saves us some time later. |
|
Gah, one of these days I'll actually remember to tag my own PR 🙄 |
|
|
Hey there, I'm lurking because I wanted to see what @carljm is up to in his first days at Astral. :) We have a similar scheme for release notes based on labels in a project I work on, and we use https://github.com/agilepathway/label-checker to set a failed check on the PR if it doesn't have one of the required labels. |
|
Hi @davisagli ! I just brought up this automation idea the other day :) But then I withdrew it; I think it would make sense for a private project, but for a project with lots of contributors like ruff, where most PR authors don't even have the rights to set labels, I think it's a bad contributor experience (and a bad reviewer experience!) for every PR to by default get the big red X until a maintainer comes along to set a label. I'd rather have the maintainers need to remember to do it. |
|
(Just my thoughts and possible solutions on mentioned issues at here)
Isn't it possible to specify this for label-checker to validate if PR author has label right or not?
I think it could be as Skipped task instead of big red X |
## Summary Came across this code while digging into the semantic model with @AlexWaygood, and found it confusing because of how it splits `push_scope` from the paired `pop_scope` (took me a few minutes to even figure out if/where we were popping the pushed scope). Since this "cleanup" is already totally split by node type, there doesn't seem to be any gain in having it as a separate "step" rather than just incorporating it into the traversal clauses for those node types. I left the equivalent cleanup step alone for the expression case, because in that case it is actually generic across several different node types, and due to the use of the common `visit_generators` utility there isn't a clear way to keep the pushes and corresponding pops localized. Feel free to just reject this if I've missed a good reason for it to stay this way! ## Test Plan Tests and clippy.
Summary
Came across this code while digging into the semantic model with @AlexWaygood, and found it confusing because of how it splits
push_scopefrom the pairedpop_scope(took me a few minutes to even figure out if/where we were popping the pushed scope). Since this "cleanup" is already totally split by node type, there doesn't seem to be any gain in having it as a separate "step" rather than just incorporating it into the traversal clauses for those node types.I left the equivalent cleanup step alone for the expression case, because in that case it is actually generic across several different node types, and due to the use of the common
visit_generatorsutility there isn't a clear way to keep the pushes and corresponding pops localized.Feel free to just reject this if I've missed a good reason for it to stay this way!
Test Plan
Tests and clippy.