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.

Sign up for GitHub

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

Jump to bottom

Add "Disable for this line" quickfix #76

Merged
merged 3 commits into from
Feb 27, 2023
Merged

Add "Disable for this line" quickfix #76

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
0ffdf42
Add "Disable for this line" quickfix
bluetech Feb 25, 2023
dc603a0
WIP: Use noqa_row
bluetech Feb 25, 2023
7f245ba
Space after comma, v0.0.TBD -> v0.0.253
bluetech Feb 25, 2023
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
2 changes: 1 addition & 1 deletion justfile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,8 +9,8 @@ install:
pip install --no-deps -r requirements-dev.txt

fmt:
ruff --fix ./ruff_lsp ./tests
black ./ruff_lsp ./tests
ruff --fix ./ruff_lsp ./tests

check:
ruff ./ruff_lsp ./tests
Expand Down
80 changes: 68 additions & 12 deletions ruff_lsp/server.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -132,11 +132,14 @@ def _parse_output_using_regex(content: str) -> list[Diagnostic]:
# {
# "code": "F841",
# "message": "Local variable `x` is assigned to but never used",
# "fixed": false,
# "location": {
# "row": 2,
# "column": 5
# },
# "end_location": {
# "row": 2,
# "column": 6
# },
# "fix": {
# "content: "",
# "location": {
Expand All @@ -148,7 +151,8 @@ def _parse_output_using_regex(content: str) -> list[Diagnostic]:
# "column: 0
# }
# },
# "filename": "/path/to/test.py"
# "filename": "/path/to/test.py",
# "noqa_row": 2
# },
# ...
# ]
Expand All @@ -167,7 +171,11 @@ def _parse_output_using_regex(content: str) -> list[Diagnostic]:
severity=_get_severity(check["code"]),
code=check["code"],
source=TOOL_DISPLAY,
data=check.get("fix"),
data=DiagnosticData(
fix=check.get("fix"),
# Available since Ruff v0.0.253.
noqa_row=check.get("noqa_row"),
),
tags=_get_tags(check["code"]),
)
diagnostics.append(diagnostic)
Expand Down Expand Up @@ -195,7 +203,8 @@ def _get_severity(code: str) -> DiagnosticSeverity:


NOQA_REGEX = re.compile(
r"(?i:# (?:(?:ruff|flake8): )?noqa)(?::\s?(?P<codes>([A-Z]+[0-9]+(?:[,\s]+)?)+))?"
r"(?i:# (?:(?:ruff|flake8): )?(?P<noqa>noqa))"
r"(?::\s?(?P<codes>([A-Z]+[0-9]+(?:[,\s]+)?)+))?"
)
CODE_REGEX = re.compile(r"[A-Z]{1,3}[0-9]{3}")

Expand Down Expand Up @@ -253,6 +262,11 @@ class Fix(TypedDict):
end_location: Location


class DiagnosticData(TypedDict):
fix: Fix | None
noqa_row: int | None


@LSP_SERVER.feature(
TEXT_DOCUMENT_CODE_ACTION,
CodeActionOptions(
Expand Down Expand Up @@ -326,7 +340,8 @@ def code_action(params: CodeActionParams) -> list[CodeAction] | None:
diagnostic
for diagnostic in params.context.diagnostics
if diagnostic.source == "Ruff"
and diagnostic.data is not None
and cast(DiagnosticData, diagnostic.data)["fix"]
is not None
],
),
]
Expand Down Expand Up @@ -391,7 +406,8 @@ def code_action(params: CodeActionParams) -> list[CodeAction] | None:
diagnostic
for diagnostic in params.context.diagnostics
if diagnostic.source == "Ruff"
and diagnostic.data is not None
and cast(DiagnosticData, diagnostic.data)["fix"]
is not None
],
),
)
Expand All @@ -400,9 +416,8 @@ def code_action(params: CodeActionParams) -> list[CodeAction] | None:
if not params.context.only or CodeActionKind.QuickFix in params.context.only:
for diagnostic in params.context.diagnostics:
if diagnostic.source == "Ruff":
if diagnostic.data is not None:
fix = cast(Fix, diagnostic.data)

fix = cast(DiagnosticData, diagnostic.data)["fix"]
if fix is not None:
title: str
if fix.get("message"):
title = f"Ruff ({diagnostic.code}): {fix['message']}"
Expand All @@ -416,9 +431,50 @@ def code_action(params: CodeActionParams) -> list[CodeAction] | None:
title=title,
kind=CodeActionKind.QuickFix,
data=params.text_document.uri,
edit=_create_workspace_edit(
document, cast(Fix, diagnostic.data)
),
edit=_create_workspace_edit(document, fix),
diagnostics=[diagnostic],
),
)

# Add "Disable for this line" for every diagnostic.
charliermarsh marked this conversation as resolved.
Show resolved Hide resolved
if not params.context.only or CodeActionKind.QuickFix in params.context.only:
for diagnostic in params.context.diagnostics:
if diagnostic.source == "Ruff":
noqa_row = cast(DiagnosticData, diagnostic.data)["noqa_row"]
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 this needs to be more defensive, since this key could be absent for older versions of Ruff (as opposed to being None).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The DiagnosticData is created by the LSP, when I create it I use noqa_row=check.get("noqa_row") so I think it's OK?

Copy link
Member

Choose a reason for hiding this comment

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

Ah you're right!

if noqa_row is not None:
line = document.lines[noqa_row - 1].rstrip("\r\n")
match = NOQA_REGEX.search(line)
# `foo # noqa: OLD` -> `foo # noqa: OLD,NEW`
if match and match.group("codes") is not None:
codes = match.group("codes") + f", {diagnostic.code}"
start, end = match.start("codes"), match.end("codes")
new_line = line[:start] + codes + line[end:]
# `foo # noqa` -> `foo # noqa: NEW`
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 this case might be impossible... since if a line has foo # noqa, the error will by definition be suppressed, right? And so this action wouldn't appear?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I managed it using the "useless noqa" lint :)

elif match:
end = match.end("noqa")
new_line = line[:end] + f": {diagnostic.code}" + line[end:]
# `foo` -> `foo # noqa: NEW`
else:
new_line = f"{line} # noqa: {diagnostic.code}"
fix = Fix(
content=new_line,
message=None,
location=Location(
row=noqa_row,
column=0,
),
end_location=Location(
row=noqa_row,
column=len(line),
),
)

actions.append(
CodeAction(
title=f"Ruff: Disable {diagnostic.code} for this line",
kind=CodeActionKind.QuickFix,
data=params.text_document.uri,
edit=_create_workspace_edit(document, fix),
diagnostics=[diagnostic],
),
)
Expand Down
26 changes: 18 additions & 8 deletions tests/test_server.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,8 @@


class TestServer(unittest.TestCase):
maxDiff = None # noqa: N815

def test_linting_example(self):
logging.info(TEST_FILE_PATH)
contents = TEST_FILE_PATH.read_text()
Expand Down Expand Up @@ -54,10 +56,13 @@ def _handler(params):
"end": {"line": 0, "character": 10},
},
"data": {
"content": "",
"message": "Remove unused import: `sys`",
"location": {"row": 1, "column": 0},
"end_location": {"row": 2, "column": 0},
"fix": {
"content": "",
"message": "Remove unused import: `sys`",
"location": {"row": 1, "column": 0},
"end_location": {"row": 2, "column": 0},
},
"noqa_row": None,
},
"message": "`sys` imported but unused",
"severity": 2,
Expand All @@ -70,6 +75,7 @@ def _handler(params):
"start": {"line": 2, "character": 6},
"end": {"line": 2, "character": 7},
},
"data": {"fix": None, "noqa_row": None},
"message": "Undefined name `x`",
"severity": 2,
"code": "F821",
Expand Down Expand Up @@ -122,10 +128,13 @@ def _handler(params):
"end": {"line": 0, "character": 10},
},
"data": {
"content": "",
"message": "Remove unused import: `sys`",
"location": {"row": 1, "column": 0},
"end_location": {"row": 2, "column": 0},
"fix": {
"content": "",
"message": "Remove unused import: `sys`",
"location": {"row": 1, "column": 0},
"end_location": {"row": 2, "column": 0},
},
"noqa_row": None,
},
"message": "`sys` imported but unused",
"severity": 2,
Expand All @@ -138,6 +147,7 @@ def _handler(params):
"start": {"line": 2, "character": 6},
"end": {"line": 2, "character": 7},
},
"data": {"fix": None, "noqa_row": None},
"message": "Undefined name `x`",
"severity": 2,
"code": "F821",
Expand Down