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

Nickel multiline strings aren't formatted #654

Open
thufschmitt opened this issue Nov 17, 2023 · 3 comments
Open

Nickel multiline strings aren't formatted #654

thufschmitt opened this issue Nov 17, 2023 · 3 comments

Comments

@thufschmitt
Copy link
Member

Describe the bug

Nickel multiline strings are mostly ignored (and left in-place) by topiary.
More precisely, it seems that the opening delimiter is taken into account, but the content of the string and the closing delimiter aren't touched.

To Reproduce

Format a file like:

  m%"
      foo
         x
        "%

The opening m%" will be moved at the begining of the line (good), but the rest will be kept with its weird indentation.

Expected behavior

I don't have a very strong opinion on how this is formatted, but there should be some consistent formatting applied. What I use and tend to see in the wild is something similar to most delimited block, namely:

  • A multi-line string in a single line (m%"foobar"%) is kept as it is
  • A “real” multi-line string is formatted such that
    • The opening token is at the end of the previous block
    • The content of the string is indented by two spaces compared to the previous block
    • The closing delimiter is indented as the previous block

(hope that makes sense).

So the above should be formatted as:

m%"
  foo
     x
"%

Some examples of multi-line strings in the wild:

Environment

  • OS name + version:
  • Version of the code: topiary-cli 0.3.0 (79b9352)

Additional context

  • The semantic of multi-line strings mean that the indentation of their content is the indentation of the less-indented line in them. Hopefully that's possible to express with topiary 🤞
  • This also applies to symbolic strings which are essentially equivalent in that regard
@Xophmeister
Copy link
Member

Strings are considered "leaf nodes" in Topiary, for the most part, meaning that no further formatting directives are applied to their contents. This is usually because their contents are format-sensitive (e.g., formatting a string will change the contents of that string, which is almost always not what is wanted). This, for example, is why interpolation sequences are not formatted, when they occur inside strings (e.g., "%{foo +1 }" will remain as-is), even though they could be. However, that would require upstream support from the Tree-sitter grammar, presuming it's possible.

IIRC, multiline and symbolic strings in Nickel are "special", in that they are indentation-aware. (I think this is what you mean by, "The semantic of multi-line strings mean that the indentation of their content is the indentation of the less-indented line in them".) I'm not sure this would be expressible in the grammar, but at the moment, it's certainly not exposed. For example, your example multiline string looks something like this to Topiary:

multiline string cst

Topiary is aware of the coordinates of each of those nodes, but you see the grammar is collapsing the string contents into a single node, so the semantics of the indentation are lost.

@thufschmitt
Copy link
Member Author

I was afraid the answer would look like that 🙃

I guess indenting the closing delimiter would still be possible though, right? (Moving it to a new line and in the right column wouldn't change the semantic since “The first and last lines are ignored if they are empty or contain only spaces”)

@Xophmeister
Copy link
Member

It may be possible 😅 ATM, the delimiters are under the node that is marked as a "forced" leaf node (str_chunks_multi). A naive moving of the leaf to the string literal nodes, to exclude the delimiters, and forcing a newline before the closing delimiter (the indentation will be fixed automatically) causes an idempotency failure in a simple test-case.

We can look into it more...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants