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
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -259,6 +259,7 @@ the following settings are supported:
| logLevel | `error` | Sets the tracing level for the extension: `error`, `warn`, `info`, or `debug`. |
| organizeImports | `true` | Whether to register Ruff as capable of handling `source.organizeImports` actions. |
| path | `[]` | Path to a custom `ruff` executable, e.g., `["/path/to/ruff"]`. |
| showSyntaxErrors | `true` | Whether to show syntax error diagnostics. _New in Ruff v0.5.0_ |

## Development

Expand Down
11 changes: 9 additions & 2 deletions ruff_lsp/server.py
Original file line number Diff line number Diff line change
Expand Up @@ -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.

if result.stdout
else []
)


def _parse_fix(content: Fix | LegacyFix | None) -> Fix | None:
Expand Down Expand Up @@ -649,7 +653,7 @@ def _parse_fix(content: Fix | LegacyFix | None) -> Fix | None:
return fix


def _parse_output(content: bytes) -> list[Diagnostic]:
def _parse_output(content: bytes, show_syntax_errors: bool) -> list[Diagnostic]:
"""Parse Ruff's JSON output."""
diagnostics: list[Diagnostic] = []

Expand Down Expand Up @@ -700,6 +704,8 @@ def _parse_output(content: bytes) -> list[Diagnostic]:
# Cell represents the cell number in a Notebook Document. It is null for normal
# Python files.
for check in json.loads(content):
if not show_syntax_errors and check["code"] is None:
continue
start = Position(
line=max([int(check["location"]["row"]) - 1, 0]),
character=int(check["location"]["column"]) - 1,
Expand Down Expand Up @@ -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.

}:
return DiagnosticSeverity.Error
else:
Expand Down
3 changes: 3 additions & 0 deletions ruff_lsp/settings.py
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,9 @@ class UserSettings(TypedDict, total=False):
ignoreStandardLibrary: bool
"""Whether to ignore files that are inferred to be part of the standard library."""

showSyntaxErrors: bool
"""Whether to show syntax error diagnostics."""

# Deprecated: use `lint.args` instead.
args: list[str]
"""Additional command-line arguments to pass to `ruff check`."""
Expand Down
Loading