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

refactor(rome_js_formatter): Introduce format_parenthesize #2718

Merged
merged 4 commits into from
Jun 17, 2022

Conversation

MichaReiser
Copy link
Contributor

@MichaReiser MichaReiser commented Jun 16, 2022

Summary

This PR replaces the usages of split_trivia where it is used to move the leading and trailing trivia of a "to be parenthesised" expression with format_parenthesize that manually formats the leading / trailing comments as part of the parentheses.

/* leading */ "string" /* trailing */;

// Becomes
/* leading */ ("string") /* trailing */;

Formatting the comments of the content as part of the parentheses requires a way to signal to the formatter that these comments have already been formatted and that it shouldn't format the comments again when formatting the "string" token. This is done by storing the "manually" formatted comments inside of the FormatState and checking for every comment if the comment has already been formatted.

The advantage of this approach is that it enables format rules to manually format comments if there's a need to do so without going over split_trivia.

New Dependencies

This PR adds a dependency on indexmap to rome_formatter. The reason to use IndexMap over HashMap is that the new map is part of the FormatState that supports taking and restoring snapshots. Thus, it is important that the data structure offers a cheap mechanism to restore the "manual comments" map to a previous state.

IndexMap (or IndexSet) offers that because it's possible to truncate the set and by doing so, remove all elements that have been added since the snapshot was taken. The built-in HashMap doesn't provide this functionality.

indexmap is only a new dependency for rome_formatter. It's already used by the parser, lsp crate and others.

Next Steps

Remove the need for split_trivia inside of group_elements.

Test Plan

cargo test. I added a few new tests verifying the new behaviour.

@MichaReiser MichaReiser temporarily deployed to aws June 16, 2022 09:32 Inactive
@github-actions
Copy link

github-actions bot commented Jun 16, 2022

@cloudflare-pages
Copy link

cloudflare-pages bot commented Jun 16, 2022

Deploying with  Cloudflare Pages  Cloudflare Pages

Latest commit: 52d366e
Status: ✅  Deploy successful!
Preview URL: https://06913405.tools-8rn.pages.dev
Branch Preview URL: https://refactor-format-parenthesize.tools-8rn.pages.dev

View logs

@MichaReiser MichaReiser marked this pull request as ready for review June 16, 2022 09:42
@MichaReiser MichaReiser temporarily deployed to aws June 16, 2022 09:43 Inactive
Copy link
Contributor

@leops leops left a comment

Choose a reason for hiding this comment

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

Would it be possible to write this helper to make it support wrapping empty nodes in parenthesis transparently, instead of having to unwrap on the first and last token everywhere it's used ?

@MichaReiser
Copy link
Contributor Author

MichaReiser commented Jun 16, 2022

Would it be possible to write this helper to make it support wrapping empty nodes in parenthesis transparently, instead of having to unwrap on the first and last token everywhere it's used ?

That's a good point. I guess we could do so for format_parenthesize. I don't see a good way to express that either both tokens must be empty or present without keeping the unwrap calls.

@MichaReiser MichaReiser temporarily deployed to aws June 16, 2022 13:17 Inactive
@MichaReiser MichaReiser requested a review from leops June 16, 2022 14:27
Copy link
Contributor

@ematipico ematipico left a comment

Choose a reason for hiding this comment

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

No particular concerns, but I would like to set on stone one styling of formatting stuff. We are mixing write! and .fmt(). I suppose the latter is less to write but I think having "one style" is better, especially for external contributors that see two things and don't know which one to use

crates/rome_formatter/src/builders.rs Outdated Show resolved Hide resolved
crates/rome_formatter/Cargo.toml Show resolved Hide resolved
crates/rome_formatter/src/lib.rs Outdated Show resolved Hide resolved
crates/rome_formatter/src/token.rs Show resolved Hide resolved
crates/rome_formatter/src/token.rs Show resolved Hide resolved
is_last_content_inline_comment,
self.style,
) {
space_token().fmt(f)?;
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: I believe we should set on a unified style across the board. Sometimes we use token("").fmt(f)? and sometimes we use write!(f, [token("")])?;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I started using fmt because it's less to write when formatting a single element and may produce slightly simpler code. But I can see the point how it might be easier for new contributors.

crates/rome_formatter/src/token.rs Show resolved Hide resolved
crates/rome_js_formatter/src/builders.rs Outdated Show resolved Hide resolved
@MichaReiser MichaReiser temporarily deployed to aws June 16, 2022 15:27 Inactive
@MichaReiser MichaReiser merged commit 7be2c4e into main Jun 17, 2022
@MichaReiser MichaReiser deleted the refactor/format-parenthesize branch June 17, 2022 09:11
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

None yet

3 participants