-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
Fix C0103 name error in main block (10766) #10824
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
base: main
Are you sure you want to change the base?
Fix C0103 name error in main block (10766) #10824
Conversation
DanielNoord
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you add a changelog entry? Other than that this LGTM
Codecov Reportβ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #10824 +/- ##
==========================================
- Coverage 95.98% 95.97% -0.02%
==========================================
Files 176 176
Lines 19594 19608 +14
==========================================
+ Hits 18808 18818 +10
- Misses 786 790 +4
π New features to boost your workflow:
|
Pierre-Sassoulas
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for opening a merge request.
Performance wise it probably make more sense to check for main block only if we're going to raise an "invalid-name", checking the ancestors of all the nodes before checking the name is going to cost a lot. What alternative did you consider ?
This comment has been minimized.
This comment has been minimized.
|
@DanielNoord Sure thing, I'll add that changelog. @Pierre-Sassoulas I didn't really consider performance at all. But we could do an 'All Caps' regex check, and then only run the new function if that doesn't match? Right now it would be looking at all variables outside a function, after this proposed change it would be looking at all lowercase variables outside of a function. It would be a small difference, but still less ancestor iterations than before. The only thing I can think of to check is if the variable is all caps. Did you have anything else in mind? |
I was thinking of moving the filtering later on in |
|
@Pierre-Sassoulas Moved the check into _check_name. |
DanielNoord
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll let @Pierre-Sassoulas finish the review here :)
|
π€ Effect of this PR on checked open source code: π€ Effect on music21: Details
The following messages are no longer emitted: Details
This comment was generated for commit eccd00c |
Type of Changes
Description
Closes #10766
Lowercase variables are now allowed inside main blocks.