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 invalid-ignore-comment rule #15094

Merged
merged 1 commit into from
Dec 23, 2024
Merged

Conversation

MichaReiser
Copy link
Member

@MichaReiser MichaReiser commented Dec 21, 2024

Summary

Red Knot skips over type: ignore and knot: ignore comments that are syntatically incorrect. This can lead to usless
wana be ignore comments lingering in the code base or confusion when users don't understand why their ignore comment doesn't work.

This PR adds a new invalid-ignore-comment rule that flags ignore comments that are simply incorrect.

Design questions

The rule flags invalid type: ignore[code code] comments even though Red Knot itself ignores the code portion of it. I think this is the correct behavior because Red Knot would ignore an invalid type: ignore comment. This way users at least know that Red Knot doesn't understand it. However, there's an argument that we shouldn't be opinionated about ignore comments from other tools.

I think the "right" solution is adding an option to configure whether the unused-ignore-comment and invalid-ignore-comment should ignore type: ignore comments. If this turns out to be a problem in the future.

Test Plan

Added mdtests

@MichaReiser MichaReiser force-pushed the micha/invalid-ignore-comment branch from 07f8973 to f1764a3 Compare December 21, 2024 17:45
@MichaReiser MichaReiser added the red-knot Multi-file analysis & type inference label Dec 21, 2024
@MichaReiser MichaReiser marked this pull request as ready for review December 21, 2024 17:49
Copy link
Contributor

@carljm carljm left a comment

Choose a reason for hiding this comment

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

Very nice!

Base automatically changed from micha/unknown-rule to main December 23, 2024 10:30
@MichaReiser MichaReiser force-pushed the micha/invalid-ignore-comment branch from f1764a3 to c12eba0 Compare December 23, 2024 10:31
@MichaReiser MichaReiser enabled auto-merge (squash) December 23, 2024 10:36
@MichaReiser MichaReiser merged commit 8d32708 into main Dec 23, 2024
20 checks passed
@MichaReiser MichaReiser deleted the micha/invalid-ignore-comment branch December 23, 2024 10:38
Copy link
Contributor

ruff-ecosystem results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

✅ ecosystem check detected no linter changes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
red-knot Multi-file analysis & type inference
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants