Skip to content
This repository has been archived by the owner on Aug 31, 2023. It is now read-only.

☂️ Formatter API improvements #1726

Closed
6 tasks done
MichaReiser opened this issue Oct 29, 2021 · 11 comments
Closed
6 tasks done

☂️ Formatter API improvements #1726

MichaReiser opened this issue Oct 29, 2021 · 11 comments
Assignees
Labels
umbrella Issue to track a collection of other issues

Comments

@MichaReiser
Copy link
Contributor

MichaReiser commented Oct 29, 2021

Part of #1718

Umbrella task to update the Formatter after we made the improvements to our parser.

  • Change formatter result type #1740

  • Rename format_raw to format_verbatim feat(rome_formatter): format_verbatim and unknown nodes #1976

  • Decision: Add helpers for formatting nodes

    • A) Implement extension methods on SyntaxResult , Option, and AstNode/ SyntaxToken to format the element (to reduce the number of try operators). You can find some ideas here but please spend some time to think about alternatives (for example, should we add helpers to the FormatResult type?).

    • B) Provide helpers to format optional tokens, for example, formatter.format_optional_token(token) -> FormatElement and overload format_token to accept an Option: format_token<T: Into<Option<Token>>? Or implement the format methods as extension methods?

      formatter.format_token(stmt.semicolon_token()).unwrap_or_else(|| token(";"));
      formatter.format_optional_token(element.comma_token());

      Implemented as part of feat(rome_formatter): retain comments in the formatter output #1916: format_token is generic over optional tokens, can take either a SyntaxToken or Option<SyntaxToken> and return a FormatElement or Option<FormatElement> respectively

  • Change the formatter to visit every token, including, and ;.

  • feat(rome_formatter): retain comments in the formatter output #1916

  • Re-evaluate CST-> CST printing? This should now be possible with explicit rules to which token trivia gets attached.

@MichaReiser MichaReiser added the umbrella Issue to track a collection of other issues label Oct 29, 2021
@MichaReiser
Copy link
Contributor Author

We probably also want to explore to codegen the union formatter implementations

@ematipico
Copy link
Contributor

We probably also want to explore to codegen the union formatter implementations

It makes sense. The only drawback is that if we create new nodes out of it, we must implement also the ToFormatElement for those nodes.

I think we could also automate that part, at a certain extent. RomeJS used to create a builder for each node. If we decided to go in the same direction, we might be able to auto generate a simple scaffolding for new nodes.

@xunilrj
Copy link
Contributor

xunilrj commented Nov 18, 2021

When finishing the trivia changes I had to decide what to do when formatting statements with problems. See https://github.com/rome/tools/pull/1801/files#r752272632

Basically we are trimming "whitespaces" around the statement; but keeping everything inside as it is.
Would be interesting to revisit this later and decide what we want to do.

@Boshen
Copy link
Contributor

Boshen commented Dec 23, 2021

Decision: Add helpers for formatting nodes

After writing a lot of boilerplate code, I vote for adding

formatter.format_token(stmt.semicolon_token()).unwrap_or_else(|| token(";"));

This is more explicit, and we gain more control on the unwrap_or_else case

@Boshen
Copy link
Contributor

Boshen commented Dec 23, 2021

Handling of "manually inserted newlines"

To align with prettier, we should collapse new lines appropriately.

As @ematipico pointed out in #1882 (comment)

I think this fix requires its own PR and its own issue, mainly because this change involves rome_rowan as well. Myself and Micha were discussing about having a new_line_trivia but this still up in the air and not sure if this is the right direction. That's why it's worth a small discussion first.

Generally, I think this logic should live inside the formatter (or even inside the printer) BUT the single nodes should not be aware of this. We might also check what old Rome was doing.

@Boshen
Copy link
Contributor

Boshen commented Dec 23, 2021

There are two long wrapping patterns that can have a common API:

  1. Wrapping long list with surrounded tokens
(aaaaaaaaaaaaaaaaaaaaaaaaaaaaaa,bbbbbbbbbbbbbbbbbbbbbbbbbbbbbb,cccccccccccccccccccccccccccccc)
[aaaaaaaaaaaaaaaaaaaaaaaaaaaaaa,bbbbbbbbbbbbbbbbbbbbbbbbbbbbbb,cccccccccccccccccccccccccccccc]
{aaaaaaaaaaaaaaaaaaaaaaaaaaaaaa,bbbbbbbbbbbbbbbbbbbbbbbbbbbbbb,cccccccccccccccccccccccccccccc}

We already have some boilerplate code:

Ok(group_elements(format_elements!(
  formatter.format_token(&self.l_brack_token()?)?,
  soft_indent(format_elements![
    join_elements(
      soft_line_break_or_space(),
      formatter.format_separated(elements)?
    ),
    trailing_comma,
  ]),
  formatter.format_token(&self.r_brack_token()?)?,
)))
  1. Wrapping long expressions with operators in between
let x = aaaaaaaaaaaaaaaaaaaaaaaaaaaaaa
  ? bbbbbbbbbbbbbbbbbbbbbbbbbbbbbb
  : cccccccccccccccccccccccccccccc;

var x = aaaaaaaaaaaaaaaaaaaaaaaaaaaaaa,
  y = bbbbbbbbbbbbbbbbbbbbbbbbbbbbbb,
  zzzzzzzzzzzzzzzzzzzzzzzzzzzzzz = c;

let x =
  aaaaaaaaaaaaaaaaaaaaaaaaaaaaaa +
  bbbbbbbbbbbbbbbbbbbbbbbbbbbbbb +
  cccccccccccccccccccccccccccccc;

@ematipico
Copy link
Contributor

The second example has a different pattern for each case, this means that we would not be able to create a utility for all of them, I think.

@ematipico
Copy link
Contributor

I am going to take a look at creating some helpers inside our formatter API

@ematipico
Copy link
Contributor

Finally: Print comments in the formatter in the format_node and format token methods

@leops is it something that you have been doing?

@leops
Copy link
Contributor

leops commented Jan 21, 2022

Yes I should probably update this because some of these tasks ended up being implemented as part of the comments PR: format_token is now generic and can be passed an Option<Token> to return an Option<FormatElement>, and the Formatter tracks that all the input tokens have been processed in debug mode

@ematipico
Copy link
Contributor

CST to CST formatting might be possible but can be tracked as separate exploration task.

The helpers will be tracked in #1996

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
umbrella Issue to track a collection of other issues
Projects
None yet
Development

No branches or pull requests

5 participants