Skip to content

Conversation

@jneem
Copy link
Member

@jneem jneem commented Sep 11, 2024

Support let blocks in nickel

Description

Nickel's master branch now has support for "let blocks", where multiple comma-separated bindings can be placed between the "let" and the "in", like

let
a = 1,
b = 2,
in
a + b

This PR introduces support for formatting let blocks, which it handles differently from single let bindings in two ways: the first binding is allowed to go on its own line, and new lines are allowed after each comma.

It would also be possible to indent the bindings, like

let
  a = 1,
  b = 2,
in
a + b

but it feels a bit strange to me to indent the bindings and not the term after "in".

Checklist

Checklist before merging:

  • CHANGELOG.md updated
  • README.md up-to-date

@yannham
Copy link
Member

yannham commented Sep 12, 2024

I vote for the second solution, which is for example what Nix formatters do (and Dhall, I think) and also is more in-line with the way we currently format multi-line let-bindings.

The rationale is that bindings are "one level deeper", but the body is the continuation of the program, and can be huge. And you often chain let-bindings a lot, so you don't want to end up with something like:

let
  a = 1,
  b = 2,
in
  let
    c = a,
    d = b,
  in
    let ...

@nbacquey nbacquey added the language: nickel Nickel formatting issues label Sep 12, 2024
@jneem jneem marked this pull request as ready for review September 23, 2024 09:40
@jneem
Copy link
Member Author

jneem commented Sep 23, 2024

Ok, I went for the indented bindings.

Btw, the update-wasm-grammars.sh script doesn't seem to work anymore, since nickel (and many other grammars) are no longer in the Cargo.lock. Maybe it should get the revisions from topiary-config/languages.ncl instead?

Copy link
Member

@Xophmeister Xophmeister left a comment

Choose a reason for hiding this comment

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

LGTM, although there are no test cases (see topiary-cli/tests/samples/{input,expected}/nickel.ncl)

@jneem
Copy link
Member Author

jneem commented Sep 24, 2024

Good call on the testing -- I've bumped into some idempotency issues, so will need to revisit this tomorrow...

@jneem
Copy link
Member Author

jneem commented Sep 25, 2024

Ok, it turned out my issues were caused by the comma in a let block inside a container being misinterpreted as a comma separating two items in the container. I fixed this by changing the container comma rule to only track commas after a container element, and not at some arbitrary position in the container.

@yannham yannham merged commit 98aa80f into main Sep 25, 2024
9 checks passed
@yannham yannham deleted the let-blocks branch September 25, 2024 12:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

language: nickel Nickel formatting issues

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants