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

refactor(rome_cli): file process #4653

Merged
merged 7 commits into from
Jul 4, 2023
Merged

refactor(rome_cli): file process #4653

merged 7 commits into from
Jul 4, 2023

Conversation

ematipico
Copy link
Contributor

@ematipico ematipico commented Jul 4, 2023

Summary

I finally had a breakthrough! It took me a bit to arrive there, but I finally managed to breakdown the logic of the files and re-use it when needed.

There were two significant issues:

  • the logic of processing a file lived in one single file, and that logic needed to consider different scenarios. This wasn't ideal because adding new logic causes more burden, and it's challenging to get it right without breaking the existing behaviour;
  • it was difficult to move code around due to the lifetime constraints we have;

I created a new wrapper called SharedTraversalOptions. This tiny struct is used to make the compiler happy and tell it that "ctx is just borrowed, and it won't escape the boundaries of the lifetimes".

I created a new WorkspaceFile, another tiny wrapper meant to coordinate the writing of a new file. For example, when applying fixes to a file, we need to:

  • write the new contents to disk;
  • update the CST of the file we have in the workspace;

This must be done because when we pass to the formatting pass, we need to apply the format, we need to have the new content of the file at end, even for the diagnostics.

I will look into it later if it's possible to optimize this part. Unfortunately, we need to keep a String around (it was like this before too).

This is a BREAKING CHANGE because now formatting diagnostics are always shown when running rome check. If you noticed, many snapshots were updated, offering more diagnostics. This is the intended feature of the rome check command.

Test Plan

I updated the existing tests and made sure that the emitted messages are correct.

@netlify
Copy link

netlify bot commented Jul 4, 2023

Deploy Preview for docs-rometools canceled.

Name Link
🔨 Latest commit 4e2dd48
🔍 Latest deploy log https://app.netlify.com/sites/docs-rometools/deploys/64a429c0ac0d4100078dae1c

@github-actions github-actions bot added A-Core Area: core A-CLI Area: CLI A-Diagnostic Area: errors and diagnostics labels Jul 4, 2023
@ematipico ematipico marked this pull request as ready for review July 4, 2023 10:22
@github-actions github-actions bot removed A-Core Area: core A-Diagnostic Area: errors and diagnostics labels Jul 4, 2023
@ematipico ematipico marked this pull request as draft July 4, 2023 10:24
@ematipico ematipico marked this pull request as ready for review July 4, 2023 10:26
@github-actions github-actions bot added the A-Project Area: project configuration and loading label Jul 4, 2023
@ematipico ematipico merged commit c4ceec3 into main Jul 4, 2023
17 checks passed
@ematipico ematipico deleted the refactor/check-file branch July 4, 2023 14:17
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A-CLI Area: CLI A-Project Area: project configuration and loading
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants