-
Notifications
You must be signed in to change notification settings - Fork 1.7k
fix: issue D401 only for non-test/property functions and methods #2071
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
Conversation
Extend test fixture to verify the targeting. Includes two "attribute docstrings" which per PEP 257 are not recognized by the Python bytecode compiler or available as runtime object attributes. They are not available for us either at time of writing, but include them for completeness anyway in case they one day are.
ed2ed3c to
49ea407
Compare
charliermarsh
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great! Let me know if any of the below is unclear.
| decorator_list, | ||
| .. | ||
| } => { | ||
| if name.starts_with("test") || name.eq("runTest") { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have a little helper for this --- cast::name from src/ast/cast.rs. We know these are functions, even though it's not encoded in the type system. (Not great, but we should definitely error anyway if we get a non-FunctionDef here.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Made use of that and cast::decorator_list in 05ec78c.
| for expr in decorator_list.iter() { | ||
| if let ExprKind::Name { id, .. } = &expr.node { | ||
| if id.eq("property") { | ||
| return; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like Pydocstyle defaults to accepting property,cached_property,functools.cached_property.
Maybe just for consistency with this "kind" of check, let's add a method to src/visibilty.rs like:
pub fn is_property(checker: &Checker, decorator_list: &[Expr]) -> bool {
decorator_list.iter().any(|expr| {
checker.resolve_call_path(expr).map_or(false, |call_path| {
call_path.as_slice() == ["", "property"] || call_path.as_slice() == ["functools", "cached_property"]
})
})
}That will check the additional decorator and ensure that it only triggers when functools is imported and track aliases and track overrides :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I actually implemented is_property in visibility.rs in my first cut of this patch, but then decided to move it back, "inlined" at the call site. I felt @property isn't actually related to visibility in the broad sense the other similar helpers there are, and thus it seemed misplaced.
But no strong opinions really, could you confirm if you'd still rather see it there and I'll refactor.
In any case, will revise this tomorrow. Thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I think that's a totally reasonable instinct, but I'd still rather see it in there. There are some other helpers in that file that aren't really related to visibility (like is_override), and I'd like to have them logically grouped for now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, done in f38b51a.
|
Looks great! Will merge when CI is green. |
|
Thanks tons. |
|
@scop Thanks for refining this! ❤️ |
[](https://renovatebot.com) This PR contains the following updates: | Package | Change | Age | Adoption | Passing | Confidence | |---|---|---|---|---|---| | [ruff](https://togithub.com/charliermarsh/ruff) | `^0.0.230` -> `^0.0.231` | [](https://docs.renovatebot.com/merge-confidence/) | [](https://docs.renovatebot.com/merge-confidence/) | [](https://docs.renovatebot.com/merge-confidence/) | [](https://docs.renovatebot.com/merge-confidence/) | --- ### Release Notes <details> <summary>charliermarsh/ruff</summary> ### [`v0.0.231`](https://togithub.com/charliermarsh/ruff/releases/tag/v0.0.231) [Compare Source](https://togithub.com/charliermarsh/ruff/compare/v0.0.230...v0.0.231) #### What's Changed - fix: issue D401 only for non-test/property functions and methods by [@​scop](https://togithub.com/scop) in [https://github.com/charliermarsh/ruff/pull/2071](https://togithub.com/charliermarsh/ruff/pull/2071) - feat: flake8-use-pathlib PTH100-124 by [@​sbrugman](https://togithub.com/sbrugman) in [https://github.com/charliermarsh/ruff/pull/2090](https://togithub.com/charliermarsh/ruff/pull/2090) - refactor: remove redundant enum by [@​sbrugman](https://togithub.com/sbrugman) in [https://github.com/charliermarsh/ruff/pull/2091](https://togithub.com/charliermarsh/ruff/pull/2091) - feat: Implement TRY201 by [@​alonme](https://togithub.com/alonme) in [https://github.com/charliermarsh/ruff/pull/2073](https://togithub.com/charliermarsh/ruff/pull/2073) - Avoid nested-if violations when outer-if has else clause by [@​charliermarsh](https://togithub.com/charliermarsh) in [https://github.com/charliermarsh/ruff/pull/2095](https://togithub.com/charliermarsh/ruff/pull/2095) - Add flake8-pie PIE804: no-unnecessary-dict-kwargs by [@​sbdchd](https://togithub.com/sbdchd) in [https://github.com/charliermarsh/ruff/pull/1884](https://togithub.com/charliermarsh/ruff/pull/1884) - Add flake8-pie PIE800: no-unnecessary-spread by [@​sbdchd](https://togithub.com/sbdchd) in [https://github.com/charliermarsh/ruff/pull/1881](https://togithub.com/charliermarsh/ruff/pull/1881) - Remove some usages of default format for expressions by [@​charliermarsh](https://togithub.com/charliermarsh) in [https://github.com/charliermarsh/ruff/pull/2100](https://togithub.com/charliermarsh/ruff/pull/2100) - docs(readme): add pypa cibuildwheel by [@​sbrugman](https://togithub.com/sbrugman) in [https://github.com/charliermarsh/ruff/pull/2107](https://togithub.com/charliermarsh/ruff/pull/2107) - refactor: Get rid of `build.rs` and other refactors by [@​not-my-profile](https://togithub.com/not-my-profile) in [https://github.com/charliermarsh/ruff/pull/2101](https://togithub.com/charliermarsh/ruff/pull/2101) - Fix outdated description of ruff's support of isort settings by [@​thomkeh](https://togithub.com/thomkeh) in [https://github.com/charliermarsh/ruff/pull/2106](https://togithub.com/charliermarsh/ruff/pull/2106) - \[`flake8-bandit`] Added Rule `S612` (Use of insecure `logging.config.listen`) by [@​saadmk11](https://togithub.com/saadmk11) in [https://github.com/charliermarsh/ruff/pull/2108](https://togithub.com/charliermarsh/ruff/pull/2108) #### New Contributors - [@​sbdchd](https://togithub.com/sbdchd) made their first contribution in [https://github.com/charliermarsh/ruff/pull/1884](https://togithub.com/charliermarsh/ruff/pull/1884) **Full Changelog**: astral-sh/ruff@v0.0.230...v0.0.231 </details> --- ### Configuration 📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined). 🚦 **Automerge**: Enabled. ♻ **Rebasing**: Whenever PR is behind base branch, or you tick the rebase/retry checkbox. 🔕 **Ignore**: Close this PR and you won't be reminded about this update again. --- - [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check this box --- This PR has been generated by [Mend Renovate](https://www.mend.io/free-developer-tools/renovate/). View repository job log [here](https://app.renovatebot.com/dashboard#github/ixm-one/pytest-cmake-presets). <!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNC4xMDkuMSIsInVwZGF0ZWRJblZlciI6IjM0LjEwOS4xIn0=--> Signed-off-by: Renovate Bot <[email protected]> Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Extend test fixture to verify the targeting.
Includes two "attribute docstrings" which per PEP 257 are not recognized by the Python bytecode compiler or available as runtime object attributes. They are not available for us either at time of writing, but include them for completeness anyway in case they one day are.
Rust newbie here, review with a fine comb greatly appreciated :)