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

Support local and dynamic class- and static-method decorators #8592

Merged
merged 2 commits into from
Nov 10, 2023

Conversation

ThiefMaster
Copy link
Contributor

Summary

This brings ruff's behavior in line with what pep8-naming already does and thus closes #8397.

I had initially implemented this to look at the last segment of a dotted path only when the entry in the *-decorators setting started with a ., but in the end I thought it's better to remain consistent w/ pep8-naming and doing a match against the last segment of the decorator name in any case.

If you prefer to diverge from this in favor of less ambiguity in the configuration let me know and I'll change it so you would need to put e.g. .expression in the classmethod-decorators list.

Test Plan

Tested against the file in the issue linked below, plus the new testcase added in this PR.

@ThiefMaster ThiefMaster force-pushed the n80x-decorators-whitelist branch from 335c989 to bc25465 Compare November 9, 2023 23:28
@ThiefMaster ThiefMaster force-pushed the n80x-decorators-whitelist branch from bc25465 to c80e0b2 Compare November 9, 2023 23:40
Copy link
Contributor

github-actions bot commented Nov 9, 2023

ruff-ecosystem results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

✅ ecosystem check detected no linter changes.

@charliermarsh
Copy link
Member

I went back and forth on this a bit but ultimately I think it makes sense in the interest of pragmatism and matching user expectation.

@charliermarsh charliermarsh enabled auto-merge (squash) November 10, 2023 02:00
Copy link
Member

@charliermarsh charliermarsh left a comment

Choose a reason for hiding this comment

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

Thanks @ThiefMaster! I just modified the PR to break the logic out into standalone functions, and we also now always run the match checks rather than only if it's not an import.

@charliermarsh charliermarsh added the configuration Related to settings and configuration label Nov 10, 2023
@charliermarsh charliermarsh changed the title Fix {class,static}method-decorators for dotted names Support local and dynamic class- and static-method decorators Nov 10, 2023
@charliermarsh charliermarsh enabled auto-merge (squash) November 10, 2023 02:01
@charliermarsh charliermarsh merged commit 4ebd0bd into astral-sh:main Nov 10, 2023
16 checks passed
@ThiefMaster ThiefMaster deleted the n80x-decorators-whitelist branch November 10, 2023 08:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
configuration Related to settings and configuration
Projects
None yet
Development

Successfully merging this pull request may close these issues.

N805 not compatible with sqlalchemy hybrids
2 participants