ruff icon indicating copy to clipboard operation
ruff copied to clipboard

Split `add_noqa` process into distinctive edit generation and edit application stages

Open snowsignal opened this issue 9 months ago • 2 comments

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 avatar May 03 '24 16:05 snowsignal

ruff-ecosystem results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

✅ ecosystem check detected no linter changes.

github-actions[bot] avatar May 03 '24 16:05 github-actions[bot]

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.

MichaReiser avatar May 03 '24 16:05 MichaReiser

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

charliermarsh avatar May 03 '24 19:05 charliermarsh

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?

charliermarsh avatar May 03 '24 20:05 charliermarsh

@charliermarsh Sure, I can do that 😄

snowsignal avatar May 04 '24 01:05 snowsignal

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.

snowsignal avatar May 04 '24 01:05 snowsignal

@charliermarsh Sure, I can do that 😄

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

charliermarsh avatar May 04 '24 02:05 charliermarsh

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

snowsignal avatar May 08 '24 17:05 snowsignal

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 avatar May 10 '24 23:05 snowsignal