-
Notifications
You must be signed in to change notification settings - Fork 664
feature(rome_js_formatter): Inline comment formatting #2701
Conversation
61d7726
to
8d1fc90
Compare
Don't insert spaces for inline comments after a group start token or before a group end token.
8d1fc90
to
fd9726b
Compare
Deploying with Cloudflare Pages
|
} | ||
|
||
#[derive(Copy, Clone, Debug, Eq, PartialEq)] | ||
pub struct Empty; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should use format_removed
instead
@@ -13,5 +13,5 @@ Indent style: Tab | |||
Line width: 80 | |||
Quote style: Double Quotes | |||
----- | |||
function foo([foo, /* not used */ , /* not used */ ]) {} | |||
function foo([foo, /* not used */ , /* not used */]) {} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The space before the ,
is because of the array hole
content: Argument::new(content), | ||
} | ||
} | ||
|
||
#[derive(Copy, Clone)] | ||
pub struct Comment<'a, Context> { | ||
pub struct FormatComment<'a, Context> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I renamed this to avoid naming conflicts with the newly introduced Comment
struct that stores comments.
|
||
#[derive(Eq, PartialEq, Debug, Copy, Clone)] | ||
pub struct LastTokenKind { | ||
kind_type: TypeId, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wanted to avoid the formatter needing a L
type parameter. I believe this should be safe even in cases where we mix different languages because it then always inserts a leading space before the next token (same as for skipped token trivia)
fd9726b
to
053eb5a
Compare
#[derive(Copy, Clone, Debug)] | ||
pub struct JsCommentStyle; | ||
|
||
impl CommentStyle for JsCommentStyle { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ideally, it would be possible that rome_formatter
defines a new Language
trait that extends rome_rowan::Language
and adds these methods to it. Unfortunately, this isn't possible without setup because rome_js_formatter
isn't able to implement that trait because of the orphan rule: Both the trait and the JsLanguage
struct are foreign to the crate.
There was a problem hiding this 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 add new tests to show us the new changes? This is more needed, I think, especially because we decided to diverge.
crates/rome_formatter/src/token.rs
Outdated
S: CommentStyle + 'static, | ||
{ | ||
fn fmt(&self, f: &mut Formatter<C>) -> FormatResult<()> { | ||
let has_trailing_inline_comment = f.state().has_trailing_inline_comment(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I understood correctly, here we check if the next token has comments?
EDIT: actually, no. The next comment says if previous token has comments (my initial thoughts), but the description of the API says has_trailing_inline_comment
says:
Returns `true` if there's a trailing inline comment before the next token or the next token's comments.
Maybe we should revisit the documentation or align the comments, so there's no confusion. Or call the API with a different name? previous_token_has_trailing_inline_comment
? Just a suggestion
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is good feedback and you're right, it's the previous token. The logic behind this is non-trivial. What it tracks is whether the last written comment requires a space before whatever content gets written next (that can be a comment, skipped token, or a regular token). Let's see if I can improve the documentation somehow.
I added a few more tests to the |
This PR improves the formatting of inline comments (
/* comment */
) so that it doesn't insert any space after a group start token (any of(
,[
,{
) nor before a group end token (any of)
,}
,]
,,
,;
,EOF
,.
).The implementation introduces new helpers to format tokens:
format_inserted
: To format a token that isn't present in the source code. For example, when adding parentheses around an expression. Use this overtoken
format_replaced
: To replace the content of a token with some other contentformat_removed
: To remove a token but still format the tokens triviaformat_only_if_breaks
: A combination offormat_replaced
andformat_removed
. Used in the cases where a token should be removed if its enclosing group doesn't break but otherwise gets formatted as is (or with slightly different content). Commonly used when formatting the trailing separator of a list.The PR introduces two new formatter states:
This PR moves the comment formatting to
rome_formatter
because there's now nothing JS-specific about formatting comments.Closes #2447
Differences to Prettier's formatting'
I decided to diverge from Prettier for the following case:
Prettier omits whitespace for inline comments before a
(
token but it doesn't do so for[
or{
in which case prettier prints a single whitespace too. In my view, inline comments before start group tokens should either all be separated from the token by a whitespace or the whitespace is omitted for all of them. That's why I didn't align Rome with Prettier's behaviour in this case.Test Plan
cargo test
.Reviewed the changed snapshot, added a few new tests
Increases compatibility by ~0.6% (from 72.65% to 73.25%)