Improve nested if printing #20

patrick91 · 2018-01-14T09:33:00Z

Currently transforms this:

if foo:
    foo()
elif z:
    if bar:
        bar()
    if baz:
        baz()
    elif x:
        bax()
else:
    if bar:
        bar()
    if baz:
        baz()
    elif x:
        bax()

to this, which is broken:

if foo:
  foo()
elif z:
  if bar:
    bar()

  if baz:
    baz()
  elif x:
    bax()
else:
  elif bar:
    bar()
  elif baz:
    baz()
  elif x:
    bax()

Almost there :)

taion · 2018-01-14T18:51:03Z

src/printer/index.js

        return concat(path.map(printElseBody, "orelse"));
      }
      return concat([
        hardline,
        "else:",
-        concat(path.map(printElseBody, "orelse"))
+        indent(concat(path.map(printElseBody, "orelse")))


Just don't use printElseBody here and you should be good. You only need special-case handling for the elif case.

taion · 2018-01-14T20:19:40Z

I will say that both this and the multi-with thing in Python 2 make me understand better what yapf does.

The Python AST has a much looser mapping to the tokens than the JS AST does. The following pieces of code are not just equivalent – they're actually syntactically identical:

if foo:
    foo()
elif bar:
    bar()
else:
    baz()

if foo:
    foo()
else:
    if bar:
        bar()
    else:
        baz()

The same is not true of the analogous code in JS.

Fortunately asttokens means we do actually have the token information available here.

taion · 2018-01-14T20:21:09Z

Calling it "fucked up" is a bit much, though. It's an AST. It doesn't have to reflect all the syntactic sugar used in the code.

patrick91 · 2018-01-14T20:50:43Z

@taion thanks for that I noticed it as well, anyway, before seeing your comment I've tried a different approach, can you have a look at the code now? :)

I've tested with your example code and it prints always the same:

if foo:
  foo()
elif bar:
  bar()
else:
  baz()

I'm not sure if we should change this :)

taion · 2018-01-14T20:49:27Z

src/printer/index.js

@@ -189,10 +150,29 @@ function printIf(path, print) {
    indent(concat([hardline, printBody(path, print)]))
  ];

-  const orElse = printOrElse(path);
+  while (n.orelse && n.orelse.length === 1 && n.orelse[0].ast_type === "If") {
+    n = n.orelse[0];


maybe just something like:

parts.push(concat(path.map(printElif, 'orelse')));

and then have printElif deal with this if clause, or even like a printIfBodyAndOrElse

I'm not sure I'm understanding what you mean here

taion · 2018-01-14T20:51:21Z

src/printer/index.js

+  if (n.orelse && n.orelse.length > 0) {
+    parts.push(hardline, "else:");
+
+    for (let i = 0; i < n.orelse.length; i++) {


can't you just do

parts.push(indent(concat(path.map(print, 'orelse'))));

for some reason it doesn't work, it is like it is missing the indent call:

if foo: foo() elif bar: bar() else:baz()

you need the hardline

Why are we doing a manual for loop here rather than just re-using our printBody function, like we do the other times in this function?

taion · 2018-01-14T20:55:14Z

I opened #22 to track some of these ambiguities. I don't think I've ever intentionally written an else: with only a single if in there, so this is probably fine, but it's worth thinking about.

patrick91 · 2018-01-14T20:57:36Z

@taion thanks for that, pretty helpful! I'll have a look at your comments soon (I'm running out of battery)

patrick91 · 2018-02-20T21:54:15Z

@FuegoFro would mind having a look at this one? I think your input will be super helpful :)

FuegoFro

Agree that this is a super interesting problem and a look at how Python's AST handles this! I made some suggestions inline :) The main concern I have is that this is going to morph the code into something that's syntactically different but parses/desugars to the same AST.

FuegoFro · 2018-02-21T03:18:22Z

tests/python_elif_multi_body/elif_multi_body.py

+    if bar:
+        bar()
+    if baz:
+        baz()


It'd be great to see some tests that have elif in them! Maybe something like

if foo: foo() elif bar: bar() else: if baz: baz() elif garply: garply() else: qux()

Maybe also a simple case like:

if a: a() else: if b: b()

My guess is that to make the above cases work (and not, for instance, collapse the end of the second case into elif b) we're going to need to modify the printer function to use n.source (probably looking at what it starts with) to determine if the orelse is an else or elif.

FuegoFro · 2018-02-21T05:33:53Z

src/printer/index.js

+  if (n.orelse && n.orelse.length > 0) {
+    parts.push(hardline, "else:");
+
+    for (let i = 0; i < n.orelse.length; i++) {


Why are we doing a manual for loop here rather than just re-using our printBody function, like we do the other times in this function?

FuegoFro · 2018-02-21T05:38:57Z

src/printer/index.js

@@ -189,10 +150,29 @@ function printIf(path, print) {
    indent(concat([hardline, printBody(path, print)]))
  ];

-  const orElse = printOrElse(path);
+  while (n.orelse && n.orelse.length === 1 && n.orelse[0].ast_type === "If") {


nit: wouldn't hurt to have a comment or two in here :) Maybe something along the lines of this?

# "Handle the nested `elif`, which are represented as `else: if...` in the AST."

Up to you though :)

FuegoFro · 2018-02-21T05:41:10Z

src/printer/index.js

@@ -189,10 +150,29 @@ function printIf(path, print) {
    indent(concat([hardline, printBody(path, print)]))


nit: If you wanted, you could update this function to use the indentConcat function, which reduces the number of parens you need :D

patrick91 · 2018-05-05T03:18:21Z

Closing in favour of #85

Improve nested if printing

a18f469

patrick91 mentioned this pull request Jan 14, 2018

Fix elif with multiple body #7

Merged

taion reviewed Jan 14, 2018

View reviewed changes

Use iterative approach to print if

83f14fc

taion reviewed Jan 14, 2018

View reviewed changes

patrick91 requested a review from FuegoFro February 20, 2018 21:54

FuegoFro suggested changes Feb 21, 2018

View reviewed changes

patrick91 mentioned this pull request May 5, 2018

Fix nested ifs #85

Merged

patrick91 closed this May 5, 2018

patrick91 deleted the feature/fix-nested-multi-if branch May 5, 2018 03:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve nested if printing #20

Improve nested if printing #20

patrick91 commented Jan 14, 2018 •

edited

Loading

taion Jan 14, 2018

taion commented Jan 14, 2018 •

edited

Loading

taion commented Jan 14, 2018

patrick91 commented Jan 14, 2018

taion Jan 14, 2018

patrick91 Jan 14, 2018

taion Jan 14, 2018

patrick91 Jan 14, 2018

taion Jan 14, 2018

FuegoFro Feb 21, 2018

taion commented Jan 14, 2018

patrick91 commented Jan 14, 2018

patrick91 commented Feb 20, 2018

FuegoFro left a comment

FuegoFro Feb 21, 2018

FuegoFro Feb 21, 2018

FuegoFro Feb 21, 2018

FuegoFro Feb 21, 2018

patrick91 commented May 5, 2018

		@@ -189,10 +150,29 @@ function printIf(path, print) {
		indent(concat([hardline, printBody(path, print)]))

Improve nested if printing #20

Improve nested if printing #20

Conversation

patrick91 commented Jan 14, 2018 • edited Loading

Choose a reason for hiding this comment

taion commented Jan 14, 2018 • edited Loading

taion commented Jan 14, 2018

patrick91 commented Jan 14, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

taion commented Jan 14, 2018

patrick91 commented Jan 14, 2018

patrick91 commented Feb 20, 2018

FuegoFro left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

patrick91 commented May 5, 2018

patrick91 commented Jan 14, 2018 •

edited

Loading

taion commented Jan 14, 2018 •

edited

Loading