-
Notifications
You must be signed in to change notification settings - Fork 664
refactor(rome_js_formatter): Track token offsets instead of tokens #2481
Conversation
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.
Before we had the message "You tried to print the token '{:?}' twice, and this is not valid" which was somehow descriptive and helped what went wrong.
Now some message are different, which makes sense. Although they don't provide much context sometimes. Would you mind expand this section , explaining how a developer is supposed to do once they find these new messages?
I removed the |
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.
Way better! Thank you!
The `Formatter` tracks all formatted token in debug builds to catch if the same token gets formatted twice or not formatted at all. The current implementation tracks the `JsSyntaxToken`s. The downside of doing this is that a language agnostic implementation needs to track `SyntaxToken<L>` which: * has the consequence that `Formatter` must be generic over `L` * which in turn has the consequence that `Format` must be generic over `L` This PR changes the node tracking to instead only track the absolute offsets of the formatted nodes rather than holding on to the node itself. This removes the need for a generic `Language` parameter and provides the same functionality. ## Test * Commented out the `track_replaced` call in `format_replaced` and verified that some tests now panic with a message that token X wasn't formatted. * Added one an additional `track_replaced` call to `format_replaced` and verified that tests are now failing with a message saying that the token X was formatted twice.
2736b5a
to
1a087ec
Compare
Parser conformance results on ubuntu-latestjs/262
jsx/babel
ts/babel
ts/microsoft
|
The
Formatter
tracks all formatted tokens in debug builds to catch if the same token gets formatted twice or not formatted at all.The current implementation tracks the
JsSyntaxToken
s. The downside of doing this is that a language-agnostic implementation needs to trackSyntaxToken<L>
which:Formatter
must be generic overL
Format
must be generic overL
This PR changes the node tracking to instead only track the absolute offsets of the formatted nodes rather than holding on to the node itself. This removes the need for a generic
Language
parameter and provides the same functionality.A next step is to move out all
format_
functions from theFormatter
and put them into standalone functions / extension traits.Test
track_replaced
call informat_replaced
and verified that some tests now panic with a message that token X wasn't formatted.track_replaced
call toformat_replaced
and verified that tests are now failing with a message saying that the token X was formatted twice.