Skip to content
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

How do we define the indentation to remove? #13

Closed
shicks opened this issue Sep 17, 2020 · 11 comments
Closed

How do we define the indentation to remove? #13

shicks opened this issue Sep 17, 2020 · 11 comments
Labels
question Further information is requested

Comments

@shicks
Copy link
Contributor

shicks commented Sep 17, 2020

It's worth considering using the common whitespace prefix (including the space before the close delimiter) to define the margin, rather than only the first line.

Pros

  • Same as Java proposal, which (assuming the latter is accepted) would decrease some cognitive overhead from every language doing something different
  • Removes the undefined behavior from the case where the first line is indented more than other lines

Cons

  • Would require changing all the examples, since the start of the closing delimiter would now need to line up with the margin, rather than being outdented one level
@jridgewell
Copy link
Member

I agree that we should use the common whitespace among all lines.

However, I solved it a bit differently in string-dedent: Whitespace-only lines are ignored from the common prefix. This includes the lead line (the likely empty portion from the opening ` to the \n) and the last line (from the last \n to the closing `). Following Python's style, I instead make every whitespace-only line empty.

@shicks
Copy link
Contributor Author

shicks commented Sep 18, 2020

That's a good thought. It would be unfortunate for trailing whitespace to have an impact.

@michaelficarra
Copy link
Member

In my opinion, whitespace-only lines (which includes empty lines) should not be considered when determining the common whitespace prefix.

@jridgewell
Copy link
Member

@michaelficarra: Do you have an opinion on what should happen to whitespace only lines? Should we leave them as is (un-dedented), should we try to dedent them, or maybe should we empty them?

@michaelficarra
Copy link
Member

They should be dedented.

@jridgewell
Copy link
Member

jridgewell commented Sep 23, 2020

As an example here, what if the common indentation were determined to be two space chars. And, on a whitespace-only line, there are two tab chars. Should we still dedent the whitespace only line?

const string = ```
\x20\x20Should this whitespace only line
\x09\x09
\x20\x20be dedented?
```;

I think if we ignore whitespace-only lines from the common indentation, we should either ignore them when dedenting or always empty them out.

@michaelficarra
Copy link
Member

I guess I'm fine with not touching them, but my expectation would be that they would be dedented. In the case where they do not share any of the common indentation (such as the one you give), they wouldn't be touched.

@jridgewell jridgewell added the question Further information is requested label Sep 29, 2020
@jridgewell
Copy link
Member

jridgewell commented Sep 29, 2020

Another option that we could do is to use the indentation of the closing line:

{
  const str = ```
    I will contain two leading space after dedenting
  ```; // < This is the closing line

  const str = ```
    I will contain no leading space after dedenting
    ```; // < This is the closing line
}

This would allow us to skip a mildly expensive "common indentation" calculation. We could place additional requirements, like all lines are required to contain the same indentation (though we should still determine what to do with empty lines).

@jridgewell jridgewell changed the title Define margin by common whitespace prefix rather than first line? How do we define the indentation to remove? Sep 29, 2020
@shicks
Copy link
Contributor Author

shicks commented Sep 29, 2020

Using the closing line's indentation would be a reasonable approach.

Requiring the same indentation is a non-starter if I understand it correctly - wouldn't that prevent using it for anything where the string itself should have its own indentation? I.e. embedded source code would be impossible to pretty print.

@jridgewell
Copy link
Member

jridgewell commented Sep 29, 2020

wouldn't that prevent using it for anything where the string itself should have its own indentation? I.e. embedded source code would be impossible to pretty print.

You'd be allowed to have more indentation, not less. Eg, if the closing line had 2 spaces, every line must have at least 2 spaces to remove. If any line had 4 spaces, then only 2 would be removed. If any line had only 1 space, or like 1 space then 1 tab, it'd throw.

@jridgewell
Copy link
Member

The incubator call agreed with using a common indentation approach.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
question Further information is requested
Projects
None yet
Development

No branches or pull requests

3 participants