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

N805 not compatible with sqlalchemy hybrids #8397

Closed
ThiefMaster opened this issue Oct 31, 2023 · 2 comments · Fixed by #8592
Closed

N805 not compatible with sqlalchemy hybrids #8397

ThiefMaster opened this issue Oct 31, 2023 · 2 comments · Fixed by #8592
Labels
configuration Related to settings and configuration needs-decision Awaiting a decision from a maintainer

Comments

@ThiefMaster
Copy link
Contributor

Apologies for opening a new issue for what's basically part of #4604, but considering that this is one closed even though half of it has not been addressed at all, and I'm not getting any replies to my comment there, I think opening a new one makes sense - this is clearly a missing feature / bug, and it makes this particular check from ruff pretty much unusable for any codebase using SQLAlchemy's hybrid properties.

Basically I need ruff to treat either @*.expression as a classmethod decorator, or alternatively have a way of telling it that sqlalchemy.ext.hybrid.hybrid_property returns a normal property, but the method decorated with that property has a .expression attribute that is a classmethod decorator...


ruff.toml

select = ['N805']

[pep8-naming]
classmethod-decorators = [
    'classmethod',
    'declared_attr',
    'expression',
    'comparator',
]

ruff_sample.py

from sqlalchemy.ext.hybrid import hybrid_property


class SQLAlchemyModel:
    @hybrid_property
    def foo(self):
        return not self._foo

    @foo.expression
    def foo(cls):
        return ~cls._foo

Output

$ ruff check ruff_sample.py
ruff_sample.py:10:13: N805 First argument of a method should be named `self`
Found 1 error.

Version

ruff==0.1.3
@zanieb
Copy link
Member

zanieb commented Nov 1, 2023

Perhaps we can allow wildcards in classmethod-decorators; would that address your use case?

Sorry your last issue didn't get more replies, we're a small team :)

@ThiefMaster
Copy link
Contributor Author

i think one of these solutions would be the easiest and indeed solve my issue:

  • wildcard support as you proposed, applied to the whole decorator function name
  • doing a suffix match when the entry in classmethod-decorators starts with a dot

@zanieb zanieb added configuration Related to settings and configuration needs-decision Awaiting a decision from a maintainer labels Nov 1, 2023
charliermarsh pushed a commit that referenced this issue Nov 10, 2023
## 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.
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 needs-decision Awaiting a decision from a maintainer
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants