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

[ruff 0.8] [flake8-annotations] Remove deprecated rules ANN101 and ANN102 #14384

Merged
merged 2 commits into from
Nov 19, 2024

Conversation

AlexWaygood
Copy link
Member

@AlexWaygood AlexWaygood commented Nov 16, 2024

Summary

Remove deprecated rules ANN101 and ANN102. These have been deprecated since Ruff 0.2, released in February. There are no open issues on the tracker asking us to undeprecate them.

Test Plan

  • cargo test
  • git grep ANN101, git grep ANN102, git grep MissingTypeCls and git grep MissingTypeSelf all show no results
  • Wait to see how impactful the ecosystem report shows this to be

@AlexWaygood AlexWaygood added the breaking Breaking API change label Nov 16, 2024
Copy link
Contributor

github-actions bot commented Nov 16, 2024

ruff-ecosystem results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

✅ ecosystem check detected no linter changes.

@AlexWaygood
Copy link
Member Author

All the errors in the ecosystem check are due to people having already explicitly ignored the rule in their pyproject.toml files. I think that validates removing the rule.

@AlexWaygood AlexWaygood changed the title [ruff 0.8] Remove deprecated rules ANN101 and ANN102 [ruff 0.8] [flake8-annotations] Remove deprecated rules ANN101 and ANN102 Nov 16, 2024
@AlexWaygood
Copy link
Member Author

AlexWaygood commented Nov 16, 2024

I feel like my main concern with merging this now is it will make the ecosystem report less useful for other PRs targeting the ruff-0.8 branch, since, once this is merged, Ruff just can't parse the configuration file at all for repos that explicitly list these rules in tool.ruff.lint.ignore. If we want to land this, I think I'd prefer to merge it after all other changes have made it into the ruff-0.8 branch.

@AlexWaygood AlexWaygood added the do-not-merge Do not merge this pull request label Nov 16, 2024
@MichaReiser
Copy link
Member

Landing that PR late in the 0.8 release process might not be enough because it means that we go blind on ecosystem checks after the 0.8 release until all ecosystem projects have updated their configurations (which may take a while).

I think we have to change the ecosystem script to remove individual rules.

@AlexWaygood
Copy link
Member Author

Landing that PR late in the 0.8 release process might not be enough because it means that we go blind on ecosystem checks after the 0.8 release until all ecosystem projects have updated their configurations (which may take a while).

Right. I added a step to the 0.8 plan which is "file PRs with other projects helping them to update to 0.8" after we've actually cut the release. But maybe even that won't be sufficient.

@MichaReiser
Copy link
Member

Right. I added a step to the 0.8 plan which is "file PRs with other projects helping them to update to 0.8" after we've actually cut the release. But maybe even that won't be sufficient.

I haven't checked but I worry that many projects aren't even using Ruff 0.7 at this point. That means they might have to upgrade from an arbitrary old Ruff version to 0.8, and upgrading might just not be a priority for them.

@AlexWaygood
Copy link
Member Author

Okay, so for the various affected projects:

So nearly all of those look like they should be fairly easy upgrades, and it looks like they try hard to keep their ruff pin up to date. The only exception is perhaps the pip one. But the sheer quantity of affected projects means that we should probably adopt some interim solution anyway to avoid the ecosystem check breaking for all PRs.

@Avasam
Copy link

Avasam commented Nov 18, 2024

I commented the same thing in #14383 (comment), but I think it's even more relevant here.

Making "unknown rule ignored" a warning instead of a parsing error may help keep the ecosystem checks relevant when removing a rule. This is already a feature request there: #13505

@MichaReiser
Copy link
Member

Making "unknown rule ignored" a warning instead of a parsing error may help keep the ecosystem checks relevant when removing a rule. This is already a feature request there: #13505

That's a good point. I worry that the change is a bit more work. It would require an error resilient RuleSelector for deserialization.

@MichaReiser MichaReiser removed the do-not-merge Do not merge this pull request label Nov 19, 2024
@MichaReiser MichaReiser merged commit 674409f into ruff-0.8 Nov 19, 2024
20 checks passed
@MichaReiser MichaReiser deleted the alex/remove-ann101-102 branch November 19, 2024 09:00
@MichaReiser MichaReiser mentioned this pull request Nov 19, 2024
AlexWaygood added a commit that referenced this pull request Nov 19, 2024
MichaReiser added a commit that referenced this pull request Nov 20, 2024
MichaReiser added a commit that referenced this pull request Nov 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking Breaking API change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants