Skip to content
This repository was archived by the owner on Oct 1, 2019. It is now read-only.

Improve nested if printing #20

Closed
wants to merge 2 commits into from
Closed

Improve nested if printing #20

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
a18f469
Improve nested if printing
patrick91 Jan 14, 2018
83f14fc
Use iterative approach to print if
patrick91 Jan 14, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
70 changes: 25 additions & 45 deletions src/printer/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,8 @@ const softline = docBuilders.softline;
const group = docBuilders.group;
const indent = docBuilders.indent;

const FastPath = require("prettier/src/common/fast-path");

function printPythonString(raw, options) {
// `rawContent` is the string exactly like it appeared in the input source
// code, without its enclosing quotes.
Expand Down Expand Up @@ -139,48 +141,7 @@ function printWithItem(path, print) {
}

function printIf(path, print) {
function printOrElse(path) {
const n = path.getValue();

if (n.orelse && n.orelse.length > 0) {
if (n.orelse[0].ast_type === "If") {
return concat(path.map(printElseBody, "orelse"));
}
return concat([
hardline,
"else:",
concat(path.map(printElseBody, "orelse"))
]);
}
}

function printElseBody(path) {
const n = path.getValue();

const parts = [];

if (n.ast_type === "If") {
parts.push(
concat([
hardline,
"elif ",
path.call(print, "test"),
":",
indent(concat([hardline, printBody(path, print)]))
])
);
} else {
parts.push(indent(concat([hardline, path.call(print)])));
}

const orElse = printOrElse(path);

if (orElse) {
parts.push(orElse);
}

return concat(parts);
}
let n = path.getValue();

const parts = [
"if ",
Expand All @@ -189,10 +150,29 @@ function printIf(path, print) {
indent(concat([hardline, printBody(path, print)]))
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

];

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 :)

n = n.orelse[0];
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


if (orElse) {
parts.push(orElse);
path = new FastPath(n);

parts.push(
hardline,
"elif ",
path.call(print, "test"),
":",
indent(concat([hardline, printBody(path, print)]))
);
}

if (n.orelse && n.orelse.length > 0) {
parts.push(hardline, "else:");

for (let i = 0; i < n.orelse.length; i++) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can't you just do

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you need the hardline

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

const x = n.orelse[i];
path = new FastPath(x);

parts.push(indent(concat([hardline, path.call(print)])));
}
}

return concat(parts);
Expand Down
16 changes: 16 additions & 0 deletions tests/python_elif_multi_body/__snapshots__/jsfmt.spec.js.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,11 +6,27 @@ if condition:
else:
assert something
another_statement()

if foo:
foo()
else:
if bar:
bar()
if baz:
baz()
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
if condition:
pass
else:
assert something
another_statement()

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

`;
8 changes: 8 additions & 0 deletions tests/python_elif_multi_body/elif_multi_body.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,3 +3,11 @@
else:
assert something
another_statement()

if foo:
foo()
else:
if bar:
bar()
if baz:
baz()
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.