Replace yesqa hook from pre-commit configuration with Ruff's RUF100 #8692
Conversation
Signed-off-by: jirka <jirka.borovec@seznam.cz>
📝 WalkthroughWalkthroughThe pull request removes the yesqa pre-commit hook from the configuration and replaces it with Ruff's RUF100 rule in Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes 🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Cache: Disabled due to data retention organization setting Knowledge base: Disabled due to 📒 Files selected for processing (5)
💤 Files with no reviewable changes (1)
🧰 Additional context used📓 Path-based instructions (1)**/*.py⚙️ CodeRabbit configuration file
Files:
🧬 Code graph analysis (1)monai/apps/nnunet/nnunetv2_runner.py (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (19)
🔇 Additional comments (6)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
for more information, see https://pre-commit.ci
pre-commit-ci
bot
requested review from
KumoLiu,
Nic-Ma and
ericspod
as code owners
There was a problem hiding this comment.
Pull request overview
This pull request attempts to streamline linting by replacing the standalone yesqa pre-commit hook with Ruff's equivalent RUF100 rule. However, the implementation has a critical flaw: the project still actively uses flake8 with pep8-naming and flake8-bugbear plugins, but Ruff is not configured to check the corresponding "N" and "B" rule categories.
Changes:
- Removed the
yesqahook from.pre-commit-config.yaml - Enabled
RUF100rule in Ruff configuration inpyproject.toml - Removed
noqacomments from 5 locations across 3 files
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| .pre-commit-config.yaml | Removed yesqa hook and its dependencies |
| pyproject.toml | Enabled RUF100 rule for detecting unused noqa comments |
| tests/runner.py | Removed noqa comments for N802 violations (camelCase method names) |
| monai/utils/profiling.py | Removed noqa comments for B902 and N805 violations (non-standard self parameter) |
| monai/apps/nnunet/nnunetv2_runner.py | Removed noqa comments for N814 and N801 violations (naming conventions) |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
|
|
||
| class nnUNetV2Runner: # noqa: N801 | ||
| class nnUNetV2Runner: |
There was a problem hiding this comment.
The removal of the noqa: N801 comment will cause flake8 violations. The project still uses flake8 with pep8-naming plugin (configured in setup.cfg with "N" rules selected). The class name nnUNetV2Runner violates N801 (class name should use CapWords convention), but this naming is intentional to match the nnU-Net library's naming convention. Since Ruff doesn't have the "N" rule category enabled, RUF100 won't detect this, but flake8 will still flag it.
| class nnUNetV2Runner: | |
| class nnUNetV2Runner: # noqa: N801 |
| self.timed_tests = {} | ||
|
|
||
| def startTest(self, test): # noqa: N802 | ||
| def startTest(self, test): |
There was a problem hiding this comment.
The removal of the noqa: N802 comment will cause flake8 violations. The project still uses flake8 with pep8-naming plugin (configured in setup.cfg line 183 with "N" rules selected, and listed in requirements-dev.txt). The method name startTest violates N802 (function name should be lowercase) but must use this name to override the parent class method from unittest.TextTestResult. Since Ruff doesn't have the "N" rule category enabled in pyproject.toml, RUF100 won't detect this, but flake8 will still flag it when run via runtests.sh --flake8.
| def startTest(self, test): | |
| def startTest(self, test): # noqa: N802 |
| super().startTest(test) | ||
|
|
||
| def stopTest(self, test): # noqa: N802 | ||
| def stopTest(self, test): |
There was a problem hiding this comment.
The removal of the noqa: N802 comment will cause flake8 violations. The project still uses flake8 with pep8-naming plugin (configured in setup.cfg). The method name stopTest violates N802 (function name should be lowercase) but must use this name to override the parent class method from unittest.TextTestResult. Since Ruff doesn't have the "N" rule category enabled, RUF100 won't detect this, but flake8 will still flag it.
| def stopTest(self, test): | |
| def stopTest(self, test): # noqa: N802 |
| class _Iterable: | ||
|
|
||
| def __iter__(_self): # noqa: B902, N805 pylint: disable=E0213 | ||
| def __iter__(_self): |
There was a problem hiding this comment.
The removal of the noqa: B902, N805 comment will cause flake8 violations. The project still uses flake8 with flake8-bugbear and pep8-naming plugins (configured in setup.cfg with "B" and "N" rules selected). This method intentionally uses _self instead of self as the first parameter, which violates B902 (invalid first argument for instance method) and N805 (first argument should be named 'self'). Since Ruff doesn't have the "B" or "N" rule categories enabled, RUF100 won't detect this, but flake8 will still flag these violations.
| def __iter__(_self): | |
| def __iter__(self): |
|
|
||
| import monai | ||
| from monai.apps.nnunet.utils import NNUNETMode as M # noqa: N814 | ||
| from monai.apps.nnunet.utils import NNUNETMode as M |
There was a problem hiding this comment.
The removal of the noqa: N814 comment will cause flake8 violations. The project still uses flake8 with pep8-naming plugin (configured in setup.cfg with "N" rules selected). Importing NNUNETMode as M violates N814 (camelcase imported as uppercase). Since Ruff doesn't have the "N" rule category enabled, RUF100 won't detect this, but flake8 will still flag it when run via runtests.sh --flake8.
| from monai.apps.nnunet.utils import NNUNETMode as M | |
| from monai.apps.nnunet.utils import NNUNETMode as M # noqa: N814 |
|
seems first we need to land #8694 |
1 participant
This pull request updates the project's linting configuration by removing the standalone
yesqapre-commit hook and enabling the equivalent rule inruff. This streamlines the code quality checks and reduces dependency complexity.Linting configuration updates:
yesqahook from.pre-commit-config.yaml, eliminating the need for its additional dependencies and exclusions.RUF100rule in thepyproject.tomlconfiguration forruff, which provides the same functionality asyesqafor identifying unusednoqacomments.