Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Avoid removing all contents when formatting file with syntax error #314

Merged
merged 5 commits into from
Nov 10, 2023

Conversation

zanieb
Copy link
Member

@zanieb zanieb commented Nov 9, 2023

Exploring a resolution to astral-sh/ruff-vscode#336

It looks like at v0.0.42...v0.0.43#diff-9cc81288343e2240bb3387d88ab41ebf706e40c86af24cf96bad8919a48d5cceL762-L763 we no longer exit early if stdout is empty. This pull request does not restore that logic, but rather adds a return code check and exits if there is a bad return code.

Summary

  • Add failing test case for formatting code with a syntax error
  • Track return code of ruff format
  • Do not format documents with non-zero return code from Ruff

Test Plan

@zanieb
Copy link
Member Author

zanieb commented Nov 10, 2023

Remaining work involves:

  • Determine if the lint fixes are affected and need a similar fix
  • Add logging of stdout from Ruff on bad return codes
  • Test in VS Code

@zanieb
Copy link
Member Author

zanieb commented Nov 10, 2023

We could consider applying an additional safe guard, but I'm not sure when return codes are intentionally non-zero

diff --git a/ruff_lsp/server.py b/ruff_lsp/server.py
index e1e6091..437827f 100755
--- a/ruff_lsp/server.py
+++ b/ruff_lsp/server.py
@@ -1142,7 +1142,7 @@ def _result_to_workspace_edit(
     document: Document, result: RunResult | None
 ) -> WorkspaceEdit | None:
     """Converts a run result to a WorkspaceEdit."""
-    if result is None:
+    if result is None or result.exit_code != 0:
         return None
 
     if document.kind is DocumentKind.Text:

result = await _run_format_on_document(document)
if result is None:
if result is None or result.exit_code != 0:
Copy link
Member

Choose a reason for hiding this comment

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

I think we should also add this to _lint_document_impl for good measure.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think the patch in #314 (comment) would apply to all uses

Copy link
Member

Choose a reason for hiding this comment

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

Ahh you're right, we might exit 1 on lint errors.

Copy link
Member

Choose a reason for hiding this comment

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

(In fact pretty sure we do.)

@zanieb zanieb marked this pull request as ready for review November 10, 2023 00:27
@zanieb zanieb merged commit c44634e into main Nov 10, 2023
20 checks passed
@zanieb zanieb deleted the test/syntax-error-fmt branch November 10, 2023 00:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants