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

Softline-like predicates for list/tuple/record delimiters #199

Closed
nbacquey opened this issue Jan 30, 2023 · 10 comments · Fixed by #205
Closed

Softline-like predicates for list/tuple/record delimiters #199

nbacquey opened this issue Jan 30, 2023 · 10 comments · Fixed by #205
Assignees
Milestone

Comments

@nbacquey
Copy link
Member

It is customary to add a delimiter after the last element of a list/tuple/record, when the grammar allows it and the context is multi-line: it minimizes the diff when adding an element at the end of the collection.
However, it is unusual to see one when the context is single-line.

The following two pieces of code are formatted correctly:

[1; 2; 3]
[
  1;
  2;
  3;
]

The following two, less so:

[1; 2; 3;]
[
  1;
  2;
  3
]

In order to produce only the first two outputs, we would need the ability to produce a delimiter symbol only if the context is multi-line. This would be akin to @append_empty_softline, except that we would produce a ; instead of a line break.

@aspiwack
Copy link
Member

@torhovland already built this capacity. See #67.

(also, note that in Ocaml, it won't work for tuples. I think. But it does for list and records.)

@nbacquey
Copy link
Member Author

nbacquey commented Jan 30, 2023

Unless I'm mistaken, the @append_delimiter predicate cannot distinguish between single-line and multi-line nodes, which is what we need here.

@aspiwack
Copy link
Member

Ah, this is what you meant. I think we should just add the delimiter in both cases, shouldn't we? There is no strong reason either way, but it's simpler to just add the delimiter always. If we didn't add it in the single-line setting, we would want to remove it altogether, since it normalises a micro-decision away. We could do this, but isn't it unnecessarily complex?

@aspiwack aspiwack added this to the v0.1 milestone Jan 30, 2023
@torhovland
Copy link
Member

This is a tough one. On one hand I agree with @aspiwack's point of view. On the other hand I think the first two code samples at the top look the most agreeable.

@aspiwack
Copy link
Member

Ok, here is what I suggest: we add the ; everywhere for v0.1. And we consider further refinement later. What do we think about this?

@nbacquey
Copy link
Member Author

Agreed. I'll remove this issue from v0.1 and amend #198

@nbacquey nbacquey removed this from the v0.1 milestone Jan 31, 2023
@aspiwack
Copy link
Member

Don't amend: merge #198 and make a separate PR.

@nbacquey
Copy link
Member Author

nbacquey commented Jan 31, 2023

I have reconsidered. Allowing [1; 2; 3;] breaks formatting elsewhere. Fixing it is about as costly as adding this @append_multiline_delimiter predicate, so I'm going to do that instead.

@aspiwack
Copy link
Member

I have reconsidered. Allowing [1; 2; 3;] breaks formatting elsewhere.

How so?

@nbacquey
Copy link
Member Author

nbacquey commented Jan 31, 2023

Well, it's not idempotent: the general rule is that ; must be followed by a spaced softline, and it makes sense, at least in sequences:

  my_stack#push 1;
  my_stack#push 2;
  my_stack#pop

and object signatures:

let obj_with_unit_id (obj : < nothing: unit; .. >) = obj

It means that [1; 2; 3;] would be formatted as [1; 2; 3; ].

Fixing that would mean to explicitly list all contexts in which ; must be followed by a spaced softline, and I think the query file is already big enough.

On the other hand, implementing @append_multiline_delimiter is just copy/pasting the code for @append_softline, sprinkled with a bit of @append_delimiter.

Besides, I'm really adverse to seeing [foo; ] in the code instead of [foo].

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

Successfully merging a pull request may close this issue.

3 participants