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

fix(rome_ci): apply import sorting #4305

Merged
merged 5 commits into from
Mar 17, 2023
Merged

Conversation

ematipico
Copy link
Contributor

@ematipico ematipico commented Mar 17, 2023

Summary

Closes #4300

This PR introduces a new method in our workspace called organize_imports, this method is needed because, while the import sorting is part of the analyzer, it's not something we can mix with the linter. Important sorting, in this PR, becomes an independent check like formatting and linting.

This required the creation of few new diagnostics.

I split the traversal module in smaller files, because it was becoming quite long.

FYI

I intend to refactor the process file method differently, but I'm not sure it's the right way. At the moment, we progressively run checks (fix, syntax check, import sorting, format, etc.), and we bail in case we have errors, depending on which command we're running. I was having hard time while adding this new check. Let's play it differently. My idea was to run ALL checks based on the command and then show ALL diagnostics at the end. This might cause repeated code, but it would make the logic more linear and simple to follow, without too many checks and cases to handle

Plus, this PR furtherly updates the LSP to consider the configuration and apply the organizeImports action only when it's supported by the workspace.

Test Plan

I added new test cases. Manually tested that the LSP doesn't sort the imports if the feature is disabled via configuration.

Documentation

  • The PR requires documentation
  • I will create a new PR to update the documentation

@netlify
Copy link

netlify bot commented Mar 17, 2023

Deploy Preview for docs-rometools canceled.

Name Link
🔨 Latest commit 655eb19
🔍 Latest deploy log https://app.netlify.com/sites/docs-rometools/deploys/6414a9e4ba7cd0000803269b

@github-actions github-actions bot added A-CLI Area: CLI A-Core Area: core A-Diagnostic Area: errors and diagnostics A-LSP Area: language server protocol A-Project Area: project configuration and loading labels Mar 17, 2023
@ematipico ematipico marked this pull request as ready for review March 17, 2023 17:42
@ematipico ematipico merged commit 6bd0a69 into main Mar 17, 2023
@ematipico ematipico deleted the fix/organize-imports-arch branch March 17, 2023 20:05
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A-CLI Area: CLI A-Core Area: core A-Diagnostic Area: errors and diagnostics A-LSP Area: language server protocol A-Project Area: project configuration and loading
Projects
None yet
Development

Successfully merging this pull request may close these issues.

🐛 rome check and rome ci do not report unorganized imports
1 participant