Fix nested ifs #85
Fix nested ifs #85
Conversation
There was a problem hiding this comment.
I'm so sorry I've been taking so long to look at these and that this is late! Maybe we can address some of these comments in a followup PR, or I'm happy to do a followup for some of this stuff :)
I'll try to take a look at everything by the end of this weekend :)
| return concat(parts); | ||
| if ( | ||
| parent.ast_type === "If" && | ||
| parent.orelse.length > 0 && |
There was a problem hiding this comment.
I think we'll want to check if length === 1 here to handle cases like
if foo:
return 1
else:
if bar:
bar()
return 2right? In any case, it would be nice to have a test case like this (an else with an if and something else after the if).
| } | ||
|
|
||
| const parts = [ | ||
| "if ", | ||
| type, | ||
| " ", |
There was a problem hiding this comment.
Is the intent to handle long conditions that need to wrap in a followup diff?
| parts.push( | ||
| concat([ | ||
| "else:", | ||
| indentConcat([hardline, join(hardline, path.map(print, "orelse"))]) |
There was a problem hiding this comment.
I think instead of the join we want to use printBody which, among other things, will preserve blank lines appropriately.
|
|
||
| if (orElse) { | ||
| parts.push(orElse); | ||
| if (n.orelse[0].ast_type === "If") { |
There was a problem hiding this comment.
Similar to above, I think this need to also check that n.orelse.length === 1. In fact we might want to pull this out to a is_elif function that operates on the parent If AST node (where "parent" here is n, but earlier in the function is parent). Or maybe a get_elif_node which takes in the parent and either returns the single If node in the orelse or null if it's not an elif node. That would allow us to check for get_elif_node(parent) === n above and get_elif_node(n) !== null here.
Also I'd like to see a comment here explaining why we do it this way. I had to think for a bit to understand why we can't have this n.orelse[0].ast_type === "If" logic only once. Maybe something like
If this is going to be an
elifwe can't printelse:and indent, but rather let the child printelifand indent itself.
| foo() | ||
| elif bar: | ||
| bar() | ||
| elif baz: |
There was a problem hiding this comment.
It's not clear to me that this is what we want. Certainly, if I wrote the original code and it got reformatted to this, I would be frustrated. I think it can make sense to have a single if in an else block, particularly if you're planning to extend it later.
I think to fix this we'd need to look at the source of the If node, given how the AST is desugared. In particular, I think in the above is_elif or get_elif_node function I describe, we could add a check along the lines of n.orelse[0].source.startswith("elif").
Improvement over #20