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

Add showSyntaxErrors server setting #454

Merged
merged 1 commit into from
Jun 27, 2024
Merged

Conversation

dhruvmanila
Copy link
Member

@dhruvmanila dhruvmanila commented Jun 27, 2024

Summary

This PR adds a new showSyntaxErrors server setting for astral-sh/ruff#12059 in ruff-lsp.

Test Plan

VS Code

Requires: astral-sh/ruff-vscode#504

Verified that the VS Code extension is using the bundled ruff-lsp and the debug build of ruff:

2024-06-27 08:47:49.567 [info] Server run command: /Users/dhruv/work/astral/ruff-vscode/.venv/bin/python /Users/dhruv/work/astral/ruff-vscode/bundled/tool/server.py
2024-06-27 08:47:49.820 [info] Using 'path' setting: /Users/dhruv/work/astral/ruff/target/debug/ruff
2024-06-27 08:47:49.827 [info] Inferred version 0.4.10 for: /Users/dhruv/work/astral/ruff/target/debug/ruff
2024-06-27 08:47:49.828 [info] Found ruff 0.4.10 at /Users/dhruv/work/astral/ruff/target/debug/ruff

Using the following VS Code config:

{
  "ruff.nativeServer": false,
  "ruff.path": ["/Users/dhruv/work/astral/ruff/target/debug/ruff"],
  "ruff.showSyntaxErrors": false
}

First, set ruff.showSyntaxErrors to true:
Screenshot 2024-06-27 at 08 34 58

And then set it to false:
Screenshot 2024-06-27 at 08 35 19

Neovim

Using the following Ruff server config:

require('lspconfig').ruff_lsp.setup {
  cmd = { '/Users/dhruv/work/astral/ruff-lsp/.venv/bin/ruff-lsp' },
  init_options = {
    settings = {
      path = { '/Users/dhruv/work/astral/ruff/target/debug/ruff' },
      showSyntaxErrors = true,
    },
  },
}

First, set showSyntaxErrors to true:
Screenshot 2024-06-27 at 08 28 03

And then set it to false:
Screenshot 2024-06-27 at 08 28 20

@dhruvmanila dhruvmanila marked this pull request as ready for review June 27, 2024 03:21
@@ -750,6 +756,7 @@ def _get_severity(code: str) -> DiagnosticSeverity:
"F821", # undefined name `name`
"E902", # `IOError`
"E999", # `SyntaxError`
None, # `SyntaxError` as of Ruff v0.5.0
Copy link
Member Author

Choose a reason for hiding this comment

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

This is actually required after v0.5.0.

Copy link
Member

Choose a reason for hiding this comment

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

If you say it is required, does it mean that Ruff-LSP crashes or is it because it otherwise shows syntax errors as warnings?

I guess there's not much we can do about it other than keeping to emit E999 in the JSON emitter for syntax errors. It might be worth to do so for a while.

Copy link
Member Author

Choose a reason for hiding this comment

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

No, I meant that the code field in JSON would be either:

  1. E999 for all Ruff versions before v0.5
  2. None for Ruff version v0.5 and later

Both are included in this list.

Copy link
Member Author

Choose a reason for hiding this comment

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

If you say it is required, does it mean that Ruff-LSP crashes or is it because it otherwise shows syntax errors as warnings?

Oh, I think I read your question wrong but yes otherwise it'll emit syntax errors as warnings. This is why I'm including None in this list so that it's emitted as error.

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 guess there's not much we can do about it other than keeping to emit E999 in the JSON emitter for syntax errors. It might be worth to do so for a while.

I'm not sure about that. If we do keep E999 in the JSON emitter, it should probably be the same for other emitters as well otherwise users might find it confusing.

The current change will only break if there are any other diagnostics emitted by Ruff which doesn't have a rule code and is suppose to be a warning instead. I think it'll be a quite some time before this happens and by that time I think ruff-lsp will be deprecated.

Copy link
Member

@MichaReiser MichaReiser Jun 27, 2024

Choose a reason for hiding this comment

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

Makes sense. Let's ship as is. I expect that most users are using the most recent extension anyway because they auto-update.

@MichaReiser MichaReiser requested review from snowsignal and removed request for snowsignal June 27, 2024 06:20
@@ -614,7 +614,11 @@ async def _lint_document_impl(
show_error(f"Ruff: Lint failed ({result.stderr.decode('utf-8')})")
return []

return _parse_output(result.stdout) if result.stdout else []
return (
_parse_output(result.stdout, settings.get("showSyntaxErrors", True))
Copy link
Member Author

Choose a reason for hiding this comment

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

We use settings.get so this change is backwards compatible i.e., the new ruff-lsp version can be used with an old VS Code extension version.

@dhruvmanila dhruvmanila merged commit 3b6166b into main Jun 27, 2024
20 checks passed
@dhruvmanila dhruvmanila deleted the dhruv/show-syntax-errors branch June 27, 2024 07:13
@MichaReiser MichaReiser mentioned this pull request Jun 27, 2024
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