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
Don't mistake a following if for an elif #5296
Conversation
Current dependencies on/for this PR:
This comment was auto-generated by Graphite. |
PR Check ResultsBenchmarkLinux
Windows
|
## Summary This snippet used to panic because it expected to see a comma or something similar after the `2` but met the closing parentheses that is not part of the range and panicked ```python a = { 1: (2), # comment 3: True, } ``` Originally found in https://github.com/bolucat/Firefox/blob/636a717ef025c16434997dc89e42351ef740ee6b/testing/marionette/client/marionette_driver/geckoinstance.py#L109 This snippet is also the test plan.
In the following code, the comment used to get wrongly associated with the `if False` since it looked like an elif. This fixes it by checking the indentation and adding a regression test ```python if True: pass else: # Comment if False: pass pass ``` Originally found in https://github.com/gradio-app/gradio/blob/1570b94a02d23d051ae137e0063974fd8a48b34e/gradio/external.py#L478
8010d93
to
b963c7f
Compare
d6f0cd1
to
97b027a
Compare
|
||
# Regression test for formatter panic with comment after parenthesized dict value | ||
# Originally found in https://github.com/bolucat/Firefox/blob/636a717ef025c16434997dc89e42351ef740ee6b/testing/marionette/client/marionette_driver/geckoinstance.py#L109 | ||
a = { |
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.
This fix seems unrelated to if/elif right?
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.
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.
just confirmed, it's also not part of the merge commit
CommentPlacement::Default(comment) | ||
} else { | ||
CommentPlacement::trailing(preceding, comment) | ||
let base_if_indent = |
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.
Just an idea. It may be annoying to do because of all the type narrowing that's necessary and you would need to identify the right parent body but I thought it may be worth mentioning. The way to normally detect whether it is an elif
or not... oh no.... It is not. These two programs have the same AST
if True:
pass
elif False:
pass
If(
StmtIf {
range: 0..38,
test: Constant(
ExprConstant {
range: 3..7,
value: Bool(
true,
),
kind: None,
},
),
body: [
Pass(
StmtPass {
range: 13..17,
},
),
],
orelse: [
If(
StmtIf {
range: 18..38,
test: Constant(
ExprConstant {
range: 23..28,
value: Bool(
false,
),
kind: None,
},
),
body: [
Pass(
StmtPass {
range: 34..38,
},
),
],
orelse: [],
},
),
],
},
),
if True:
pass
else:
if False:
pass
If(
StmtIf {
range: 41..91,
test: Constant(
ExprConstant {
range: 44..48,
value: Bool(
true,
),
kind: None,
},
),
body: [
Pass(
StmtPass {
range: 54..58,
},
),
],
orelse: [
If(
StmtIf {
range: 69..91,
test: Constant(
ExprConstant {
range: 72..77,
value: Bool(
false,
),
kind: None,
},
),
body: [
Pass(
StmtPass {
range: 87..91,
},
),
],
orelse: [],
},
),
],
},
),
In the following code, the comment used to get wrongly associated with the
if False
since it looked like an elif. This fixes it by checking the indentation and adding a regression testOriginally found in https://github.com/gradio-app/gradio/blob/1570b94a02d23d051ae137e0063974fd8a48b34e/gradio/external.py#L478