-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
Remove redundant condition from has_magic_trailing_comma
#4023
Conversation
The second `if` cannot be true at its execution point, because it is already covered by the first `if`. The condition `comma.parent.type == syms.subscriptlist` always holds if `closing.parent.type == syms.trailer` holds, because `subscriptlist` only appears inside `trailer` in the grammar: ``` trailer: '(' [arglist] ')' | '[' subscriptlist ']' | '.' NAME subscriptlist: (subscript|star_expr) (',' (subscript|star_expr))* [','] ```
cc @hauntsaninja - you added some of the condition in 4ebf14d, maybe you can poke a hole in my logic :) Also c4bd2e3 made a change here (removing the |
Looks like the listmaker condition was only used if preview style for that year was off. Turning on the preview style by default meant that that branch was dead. |
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.
Seems like you're correct, and diff-shades also found nothing changed by this refactor.
The branch is only relevant when There's always a risk with changes like this of unexpected breakage, so it'd be nice to get some assurance like diff-shades. If diff-shades doesn't cover it, I'll look for some projects which use |
I'll run it on our internal codebase to check. |
Simplify the
has_magic_trailing_comma
logic (it will make #3918 a bit easier to implement).The second
if
cannot be true at its execution point, because it is already covered by the firstif
. The conditioncomma.parent.type == syms.subscriptlist
always holds ifclosing.parent.type == syms.trailer
holds, becausesubscriptlist
only appears insidetrailer
in the grammar:NOTE: I split the PR to two commits, the first one is just a refactor with no logical changes, but makes the argument above easier to follow. The commits should be squashed when merging.