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

fix(rome_cli): do not pull diagnostics if linter is turned off #3171

Merged
merged 2 commits into from
Sep 7, 2022

Conversation

ematipico
Copy link
Contributor

Summary

This PR fixes #3167

Diagnostics were pulled regardless of the capability of the workspace. I added a small condition before the pulling of diagnostics.

Test Plan

I created two test cases: one where the formatter is enabled and linter disabled, one where the formatter is disabled and the linter enabled

@ematipico ematipico requested a review from leops as a code owner September 7, 2022 07:39
@ematipico ematipico temporarily deployed to netlify-playground September 7, 2022 07:39 Inactive
@netlify
Copy link

netlify bot commented Sep 7, 2022

Deploy Preview for rometools canceled.

Name Link
🔨 Latest commit b923aff
🔍 Latest deploy log https://app.netlify.com/sites/rometools/deploys/63184f49a8efc800089cf9b0

@@ -0,0 +1,24 @@
---
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This test case is valid. rome ci doesn't attempt to format files if they contain diagnostics, so I created a snippet that doesn't emit lint diagnostics.

Copy link
Contributor

@MichaReiser MichaReiser Sep 7, 2022

Choose a reason for hiding this comment

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

What's the motivation of not formatting files that have lint diagnostics? Shouldn't we show a user all errors to avoid the "Rust" experience where you fix a syntax error, to then get a compile error, to then get a borrow checker error.

This is something that frustrated me a couple of times with Rust because it results in a slow feedback cycle and I may not run rome format only to then see on CI that there are also formatting errors.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry. I think I misunderstood.

We do format and check files that have lint errors but we don't format files that have syntax errors. That makes sense.

@github-actions
Copy link

github-actions bot commented Sep 7, 2022

@ematipico ematipico requested a review from leops September 7, 2022 07:59
@ematipico ematipico temporarily deployed to netlify-playground September 7, 2022 07:59 Inactive
@ematipico ematipico merged commit dcc1d72 into main Sep 7, 2022
@ematipico ematipico deleted the fix/issue-3167 branch September 7, 2022 09:33
@anonrig
Copy link
Contributor

anonrig commented Sep 7, 2022

@ematipico When can you release a new version with this fix? This is quite an important issue

@ematipico
Copy link
Contributor Author

@ematipico When can you release a new version with this fix? This is quite an important issue

After we fix #3175, for sure. Probably tomorrow

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

rome ci does not care about rome.json
4 participants