-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Implement some rules from flake8-logging-format
#2150
Implement some rules from flake8-logging-format
#2150
Conversation
ba71617
to
109a746
Compare
I don't think I'll implement |
if checker.settings.rules.enabled(&Rule::LoggingWarn) | ||
&& matches!(logging_level, LoggingLevel::Warn) | ||
{ | ||
let mut diagnostic = Diagnostic::new(LoggingWarn, level_call_range); |
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.
One concern here is that PGH002
does the same thing:
resources/test/fixtures/flake8_logging_format/G010.py:3:1: PGH002 `warn` is deprecated in favor of `warning`
We could consider deprecating that rule, though, in favor of this more comprehensive plugin. We'd basically just redirect PGH002
to G010
.
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 agree that it makes sense to prefer the logging plugin for this rule. Is it just a matter of adding it in src/rule_redirects.rs
?
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, we'd need to delete all references to the rule (fixture files, etc.), then add the redirect to that map.
|
||
/// Check logging calls for violations | ||
pub fn logging_call(checker: &mut Checker, func: &Expr, args: &[Expr], keywords: &[Keyword]) { | ||
if let ExprKind::Attribute { value, attr, .. } = &func.node { |
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.
In src/rules/tryceratops/rules/error_instead_of_exception.rs
, I used some heuristics to detect whether the value
is that of a logger (by matching on names, like logging
, logger
, self.logger
, foo_logger
, etc.). I'd like to be consistent between that rule and here. Any opinion on what's preferable?
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 followed the plugin here to just whitelist parser
and warnings
but it probably makes sense to make this more robust with some is_logger_candidate
that incorporates your heuristic additionally. Happy to try out the refactor.
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.
That would be great, 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.
[![Mend Renovate](https://app.renovatebot.com/images/banner.svg)](https://renovatebot.com) This PR contains the following updates: | Package | Change | Age | Adoption | Passing | Confidence | |---|---|---|---|---|---| | [ruff](https://togithub.com/charliermarsh/ruff) | `^0.0.235` -> `^0.0.236` | [![age](https://badges.renovateapi.com/packages/pypi/ruff/0.0.236/age-slim)](https://docs.renovatebot.com/merge-confidence/) | [![adoption](https://badges.renovateapi.com/packages/pypi/ruff/0.0.236/adoption-slim)](https://docs.renovatebot.com/merge-confidence/) | [![passing](https://badges.renovateapi.com/packages/pypi/ruff/0.0.236/compatibility-slim/0.0.235)](https://docs.renovatebot.com/merge-confidence/) | [![confidence](https://badges.renovateapi.com/packages/pypi/ruff/0.0.236/confidence-slim/0.0.235)](https://docs.renovatebot.com/merge-confidence/) | --- ### Release Notes <details> <summary>charliermarsh/ruff</summary> ### [`v0.0.236`](https://togithub.com/charliermarsh/ruff/releases/tag/v0.0.236) [Compare Source](https://togithub.com/charliermarsh/ruff/compare/v0.0.235...v0.0.236) #### What's Changed ##### Rules - feat: implement `TRY002` and `TRY003` by [@​karpa4o4](https://togithub.com/karpa4o4) in [https://github.com/charliermarsh/ruff/pull/2135](https://togithub.com/charliermarsh/ruff/pull/2135) - Implementing `TRY400` by [@​Flowake](https://togithub.com/Flowake) in [https://github.com/charliermarsh/ruff/pull/2115](https://togithub.com/charliermarsh/ruff/pull/2115) - Implement some rules from `flake8-logging-format` (`G`) by [@​edgarrmondragon](https://togithub.com/edgarrmondragon) in [https://github.com/charliermarsh/ruff/pull/2150](https://togithub.com/charliermarsh/ruff/pull/2150) ##### Settings - Add strictness setting for `flake8-typing-imports` by [@​charliermarsh](https://togithub.com/charliermarsh) in [https://github.com/charliermarsh/ruff/pull/2221](https://togithub.com/charliermarsh/ruff/pull/2221) - Implement `exempt-modules` setting from flake8-type-checking by [@​charliermarsh](https://togithub.com/charliermarsh) in [https://github.com/charliermarsh/ruff/pull/2230](https://togithub.com/charliermarsh/ruff/pull/2230) ##### Bug fixes - flake8\_executable: Only match shebang at beginning of line by [@​andersk](https://togithub.com/andersk) in [https://github.com/charliermarsh/ruff/pull/2183](https://togithub.com/charliermarsh/ruff/pull/2183) - Don't flag B009/B010 if identifiers would be mangled by [@​sciyoshi](https://togithub.com/sciyoshi) in [https://github.com/charliermarsh/ruff/pull/2204](https://togithub.com/charliermarsh/ruff/pull/2204) - fix: --explain reporting the wrong linter by [@​not-my-profile](https://togithub.com/not-my-profile) in [https://github.com/charliermarsh/ruff/pull/2215](https://togithub.com/charliermarsh/ruff/pull/2215) - Preserve indentation when fixing via LibCST by [@​charliermarsh](https://togithub.com/charliermarsh) in [https://github.com/charliermarsh/ruff/pull/2223](https://togithub.com/charliermarsh/ruff/pull/2223) - Avoid erroneous class autofixes in indented blocks by [@​charliermarsh](https://togithub.com/charliermarsh) in [https://github.com/charliermarsh/ruff/pull/2226](https://togithub.com/charliermarsh/ruff/pull/2226) - Fix range for `try-consider-else` by [@​charliermarsh](https://togithub.com/charliermarsh) in [https://github.com/charliermarsh/ruff/pull/2228](https://togithub.com/charliermarsh/ruff/pull/2228) - Avoid flagging blind exceptions with valid logging by [@​charliermarsh](https://togithub.com/charliermarsh) in [https://github.com/charliermarsh/ruff/pull/2232](https://togithub.com/charliermarsh/ruff/pull/2232) - Avoid removing trailing comments on `pass` statements by [@​charliermarsh](https://togithub.com/charliermarsh) in [https://github.com/charliermarsh/ruff/pull/2235](https://togithub.com/charliermarsh/ruff/pull/2235) - Allow `pytest` in shebang by [@​charliermarsh](https://togithub.com/charliermarsh) in [https://github.com/charliermarsh/ruff/pull/2237](https://togithub.com/charliermarsh/ruff/pull/2237) - Wrap return-bool-condition-directly fixes in bool() by [@​charliermarsh](https://togithub.com/charliermarsh) in [https://github.com/charliermarsh/ruff/pull/2240](https://togithub.com/charliermarsh/ruff/pull/2240) #### New Contributors - [@​Flowake](https://togithub.com/Flowake) made their first contribution in [https://github.com/charliermarsh/ruff/pull/2115](https://togithub.com/charliermarsh/ruff/pull/2115) - [@​henryiii](https://togithub.com/henryiii) made their first contribution in [https://github.com/charliermarsh/ruff/pull/2200](https://togithub.com/charliermarsh/ruff/pull/2200) - [@​sciyoshi](https://togithub.com/sciyoshi) made their first contribution in [https://github.com/charliermarsh/ruff/pull/2204](https://togithub.com/charliermarsh/ruff/pull/2204) **Full Changelog**: astral-sh/ruff@v0.0.235...v0.0.236 </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:eyJjcmVhdGVkSW5WZXIiOiIzNC4xMTEuMSIsInVwZGF0ZWRJblZlciI6IjM0LjExMS4xIn0=--> Signed-off-by: Renovate Bot <bot@renovateapp.com> Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
#1850