Improve multiline printing of record types and values#7993
Improve multiline printing of record types and values#7993shulhi merged 8 commits intorescript-lang:masterfrom
Conversation
rescript
@rescript/darwin-arm64
@rescript/darwin-x64
@rescript/linux-arm64
@rescript/linux-x64
@rescript/runtime
@rescript/win32-x64
commit: |
nojaf
left a comment
nojaf
left a comment
There was a problem hiding this comment.
Can we first reach consensus with the core contributors before merging?
This sets a precedent, and I think we should have a broader discussion about the direction we want to take.
For context, see my earlier thoughts here: #7961 (comment)
| * }` -> record is written on multiple lines, break the group *) | ||
| let force_break = | ||
| e.pexp_loc.loc_start.pos_lnum < e.pexp_loc.loc_end.pos_lnum | ||
| match (spread_expr, rows) with |
There was a problem hiding this comment.
I want to pause here a bit — I don’t think we should merge this part right now.
The change around force_break introduces behaviour that depends on how the user originally formatted their code. That’s a significant precedent: it ties output formatting decisions to source‑layout heuristics.
If we want to allow this kind of stylistic inference, I believe it needs wider agreement within the core team. We should either decide as a group that this is a direction we’re comfortable with, or explicitly document why we don’t want to go that way.
If we do agree in principle, I’d also like to see a short architectural or design record capturing the rationale and boundaries for this sort of logic, so that future contributions have something concrete to refer to.
Finally, even with buy‑in, I’d suggest postponing this until after v12. We’re in the release‑candidate stage, and introducing new formatting heuristics at this point could have more ripple effects than we realise.
So in short: let’s not merge this specific part yet; let’s first get consensus and document it properly.
shulhi
force-pushed
the
improve-record-formatter-2
branch
2 times, most recently
from
a7e7b59 to
b2c1089
Compare
|
@nojaf should we get this in? I think we've made some decision on this since the latest v12 release. |
|
Seems like shulhi#2 got lost after force push? |
Didn't notice this was lost after rebasing, I'll get that in. |
| #### :memo: Documentation | ||
|
|
||
| #### :nail_care: Polish | ||
| - Formatter: Improve multiline printing of record types and values. https://github.com/rescript-lang/rescript/pull/7993 |
There was a problem hiding this comment.
Nit: add extra newline after header
| first.pld_loc.loc_start.pos_lnum < last.pld_loc.loc_end.pos_lnum | ||
| | None, Some loc, first :: _ -> | ||
| (* Check if first field is on a different line than the opening brace *) | ||
| loc.loc_start.pos_lnum < get_field_start_line first |
There was a problem hiding this comment.
Keep in mind: if we lean into this more over time, we'll probably want a small helper function so we can easily trace where we apply this heuristic and where we don't. It's fine to implement that in a later phase.
There was a problem hiding this comment.
Definitely, I'm going to tackle a similar multiline support for record and we will see if this something we can extract to a helper function.
nojaf
left a comment
nojaf
left a comment
There was a problem hiding this comment.
One could argue this is more of a feature and needs a minor to be released.
A change in the formatter is not something I would expect from a revision release.
That being said, I won't die on that hill.
I don't feel strong about this.
shulhi
force-pushed
the
improve-record-formatter-2
branch
from
5abc743 to
48062ca
Compare
Saw this too late. Would also have preferred it going into 12.1, not 12.0.2. |
4 participants
Fix #7961
The formatting of the transformed JSX forces multiline for props with this change. However, I think this is fine because users won't actually see the transformed JSX anyways.