tweag / topiary

https://topiary.tweag.io/
MIT License
558 stars 26 forks source link

Nickel: Reduce vertical space with function calls #636

Closed simonzkl closed 1 year ago

simonzkl commented 1 year ago

Is your feature request related to a problem? Please describe.

I know this is a very opinionated topic, but in its current state I feel like the Nickel formatter wastes a lot of vertical space unnecessarily. We're switching from yaml + helm to Nickel for templating our Kubernetes resources and we noticed the number of lines in our templates doubling. This makes it more difficult to grok the complete template at a glance, as everything is much more spread out.

For example let's look at how Topiary formats this expression:

foo = optional_record bar.enabled {
  ...
}

Topiary:

foo = 
  optional_record
    bar.enabled
    {
      ...
    }

Notice how the record is indented twice, once for the function name and another one for the function arguments. It also moves the function name and each argument into their own lines. This is despite the entire expression easily fitting into a single line.

If you have a lot of conditional logic in your templates, this quickly adds up.

Describe the solution you'd like

I'd be happy with any of these solutions:

foo = optional_record bar.enabled {
  ...
}

or

foo = 
  optional_record bar.enabled {
    ...
  }

Describe alternatives you've considered

This issue only covers function calls, but in general I would prefer if the Nickel formatter wasn't too strict about how you combine expressions that spread out through multiple lines. I think the nixpkgs-fmt formatter as an example strikes a very nice balance, allowing you to also combine blocks like [{ \n ... }] when it makes sense. Topiary often forces you to write every bracket on its own line, leading to heavily indented code like this:

if foo
  [
    {
      bar = 1
      ...
    }
  ]
else
  []

This could be so much more compact:

if !foo [] else [{
  bar = 1
  ...
}]

Or with a helper function:

optional_list foo [{
  bar = 1
  ...
}]
Xophmeister commented 1 year ago

Thanks for making this issue. The line-spacing of function calls, as it currently stands, is by design. Any syntactic construct in Topiary can be considered "single-line" or "multi-line". In your example:

foo = optional_record bar.enabled {
  ...
}

By virtue of the function argument, which happens to be a record, spanning multiple lines, the parent node (i.e., the function) is considered multi-line. Hence, Topiary pushes things around like so:

function-name
  argument
  argument
  argument

Topiary/Tree-sitter queries don't have the fidelity to be able to make special cases for something like "if the last argument is multi-line, then format the call-site as single-line except for the last argument". (That is, IIRC: it may be possible, but I suspect that it might be needlessly complex if it can be done.)

All that being said, your point is definitely taken. There may be things we can do to compact vertical space usage, under the advisement of the Nickel project (as you say: it's subjective!) We'll look into it...

simonzkl commented 1 year ago

Thanks! I think this is definitely something you'd want to be able to do across different languages as well. It's very common for a function's last argument to be a block of code (closure) for example.

aspiwack commented 1 year ago

The larger issue is #546 , where the examples are in Ocaml. I remember the hanging forms were also a fairly early consideration in the Ormolu formatter for Haskell. All this to mean that when we have some available brain cycle it'll be worth it to try hard to crack this problem.

Xophmeister commented 1 year ago

We've discussed this amongst both the Topiary and Nickel teams and there no strong opinion on this, either way.

There is, however, an objective argument for the liberal vertical spacing: One of the stated design goals for Topiary's formatting rules is that diffs should be minimised. That is, artefacts from formatting shouldn't introduce visual clutter when looking at changes between commits. This often necessitates syntactic forms being made line-wise mutually exclusive, so they don't interfere with each other.

For example, if we add an argument to the function call site, the change is clearer in this form:

  {
    symbol =
      function
        argument1
+       argument2
        {
          record = 123
        }
  }

...than this:

  {
!   symbol = function argument1 argument2 {
      record = 123
    }
  }

As such, we've decided to close this issue and mark it as wontfix.

However, the beauty of Topiary is that it's quite easy to make these changes yourself, by editing the formatting query appropriately. For example, to format your if example in a more compact way, I just made the following changes to topiary-queries/queries/nickel.scm:

@@ -365,18 +365,18 @@
 ;
 ; This style has precedent from the manually formatted stdlib. (An
 ; alternative style is to give the "then" token its own line.)
-(ite_expr
-  "then" @append_spaced_softline @append_indent_start
-  "else" @prepend_indent_end @prepend_spaced_softline
-)
-
-(ite_expr
-  "else" @append_spaced_softline @append_indent_start
-  t2: (term
-    ; Don't apply formatting if an "else" is followed by an "if"
-    (uni_term (ite_expr))? @do_nothing
-  ) @append_indent_end
-)
+;(ite_expr
+;  "then" @append_spaced_softline @append_indent_start
+;  "else" @prepend_indent_end @prepend_spaced_softline
+;)
+
+;(ite_expr
+;  "else" @append_spaced_softline @append_indent_start
+;  t2: (term
+;    ; Don't apply formatting if an "else" is followed by an "if"
+;    (uni_term (ite_expr))? @do_nothing
+;  ) @append_indent_end
+;)

 ;; Container Types
 ; i.e., Arrays, records (and dictionaries, vicariously) and enums
@@ -399,9 +399,9 @@
 (_
   (#scope_id! "container")
   .
-  "[" @append_empty_softline @append_indent_start @prepend_begin_scope
+  "[" @prepend_begin_scope
   (_)
-  "]" @prepend_indent_end @prepend_empty_softline @append_end_scope
+  "]" @append_end_scope
   .
 )

This may need further tweaking to cover every case, but illustrates how accessible it is to customise the formatting styles to your own needs.

simonzkl commented 1 year ago

There is, however, an objective argument for the liberal vertical spacing: One of the stated design goals for Topiary's formatting rules is that diffs should be minimised. That is, artefacts from formatting shouldn't introduce visual clutter when looking at changes between commits.

I feel like if diff minimisation was the design goal, then you would want to write every function call with arguments split into multiple lines, regardless of whether a single argument is multi-line. After all, why does diff minimisation not matter for single-line functions? Obviously that's silly, and nobody would want that to be the case. So I would argue the decision is completely arbitrary, not objectively better. Which is fine, it's a very opinionated topic. It just doesn't work for me.

Customizing formatting rules would work up to a point I suppose. Ideally I would like to have the flexibility to pick a single-line or multi-line approach depending on how long the whole expression is. I assume that's not something you can really do right now, right?

yannham commented 1 year ago

I feel like if diff minimisation was the design goal, then you would want to write every function call with arguments split into multiple lines, regardless of whether a single argument is multi-line.

I would say diff minimization is one of the goal/guiding principle. There are several of them, and they may compete. I think another design choice is that Topiary respects the user's original choice with respect to single line vs multi line expressions. I agree this one is arbitrary, and as you say, it's a very subjective topic :upside_down_face:

Customizing formatting rules would work up to a point I suppose.

I suspect you can go quite far, because the whole point of Topiary is that all the formatting is done by tree-sitter queries. So you can basically mess with the whole code of the formatter. That being said, I don't have the answer to your question about splitting lines, so I'll let Topiary developers answer.

Xophmeister commented 1 year ago

Ideally I would like to have the flexibility to pick a single-line or multi-line approach depending on how long the whole expression is. I assume that's not something you can really do right now, right?

That's right, I'm afraid. The only distinction we make is whether an expression was originally laid out by its author on a single line, or spread over many. The physical extents of the expression are not considered, at the moment.