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

Split add_noqa process into distinctive edit generation and edit application stages #11265

Merged
merged 7 commits into from May 10, 2024

Conversation

snowsignal
Copy link
Member

Summary

--add-noqa now runs in two stages: first, the linter finds all diagnostics that need noqa comments and generate edits on a per-line basis. Second, these edits are applied, in order, to the document.

A public-facing function, generate_noqa_edits, has also been introduced, which returns noqa edits generated on a per-diagnostic basis. This will be used by ruff server for noqa comment quick-fixes.

Test Plan

Unit tests have been updated.

@snowsignal snowsignal added the linter Related to the linter label May 3, 2024
Copy link
Contributor

github-actions bot commented May 3, 2024

ruff-ecosystem results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

✅ ecosystem check detected no linter changes.

@MichaReiser
Copy link
Member

Would you mind running some hyperfine benchmarks on this change. The Contributing.md explains how. I'm asking because hashing Diagnostic can be somewhat expensive.

I wonder if we should do some manual testing. What's your experience @charliermarsh with this kind of change?

I won't have time to review this today. I plan to look into it on Monday.

@charliermarsh
Copy link
Member

Is there an LSP PR that I can look at in tandem to understand how the API will be used?

crates/ruff_linter/src/noqa.rs Outdated Show resolved Hide resolved
crates/ruff_linter/src/noqa.rs Show resolved Hide resolved
crates/ruff_linter/src/noqa.rs Outdated Show resolved Hide resolved
@charliermarsh
Copy link
Member

I think --add-noqa has effectively no tests... @snowsignal, would you minding first opening a PR on main to add a few basic tests to crates/ruff/tests/lint.rs?

@snowsignal
Copy link
Member Author

@charliermarsh Sure, I can do that 😄

@snowsignal
Copy link
Member Author

Is there an LSP PR that I can look at in tandem to understand how the API will be used?

I've opened it here.

@charliermarsh
Copy link
Member

@charliermarsh Sure, I can do that 😄

Thanks, ultimately it will get this merged faster if we have some test coverage.

crates/ruff_linter/src/noqa.rs Outdated Show resolved Hide resolved
crates/ruff_linter/src/noqa.rs Outdated Show resolved Hide resolved
crates/ruff_linter/src/noqa.rs Outdated Show resolved Hide resolved
crates/ruff_linter/src/noqa.rs Outdated Show resolved Hide resolved
crates/ruff_linter/src/noqa.rs Outdated Show resolved Hide resolved
crates/ruff_linter/src/noqa.rs Outdated Show resolved Hide resolved
crates/ruff_linter/src/noqa.rs Outdated Show resolved Hide resolved
crates/ruff_linter/src/noqa.rs Outdated Show resolved Hide resolved
@snowsignal
Copy link
Member Author

I'm holding off on merging this until I investigate the root cause of this issue.

@snowsignal
Copy link
Member Author

The root of the related issue in the follow-up PR appears to be caused by noqa mappings not generated/passed through to the edit generation function. I've added a test to show that this does work correctly when the proper NoqaMapping is given.

@snowsignal snowsignal enabled auto-merge (squash) May 10, 2024 23:10
@snowsignal snowsignal merged commit 890cc32 into main May 10, 2024
19 checks passed
@snowsignal snowsignal deleted the jane/linter/noqa-edits branch May 10, 2024 23:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
linter Related to the linter
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants