-
-
Notifications
You must be signed in to change notification settings - Fork 41
Conversation
if condition: | ||
pass | ||
assert somethinganother_statement() | ||
|
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.
Is this intentional?
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.
Oh wow no. I made it not crash, I didn't realize it was completely broken... Oops!
@vjeux I've added a working version of the if printing, it is bit ugly, there's some code duplication as well, but we can fix that. Can you see if you like the approach? :) |
Your code looks good :) Code duplication is fine. I would say it’s even better than trying to come with some convoluted abstraction. The problem here is that the ast itself is fucked up, not that the printer code is bad or more complicated than it ought to be |
Thanks @vjeux I've cleaned it up a bit and merged :) |
@patrick91: btw, treat this as your project, feel free to commit as often as you want :) |
const n = path.getValue(); | ||
|
||
if (n.orelse && n.orelse.length > 0) { | ||
if (n.orelse[0].ast_type === "If") { |
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 should check that the length is exactly 1. Otherwise you transform
if foo:
foo()
else:
if bar:
bar()
if baz:
baz()
into
if foo:
foo()
elif bar:
bar()
elif baz:
baz()
And other weird things whenever an if
shows up under an else
for any reason.
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.
Good catch! I've submitted an initial PR to fix this: #20
Fixes #4