chore(linters): Introduce ruff and fix issues #831
Conversation
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe changes update linting configurations and perform code cleanup. The pre-commit configuration now includes new repositories for Hadolint and Bash linters with explicit ignore rules, while older configurations were removed. A new Changes
Suggested reviewers
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (11)
💤 Files with no reviewable changes (1)
✅ Files skipped from review due to trivial changes (6)
⏰ Context from checks skipped due to timeout of 90000ms (12)
🔇 Additional comments (17)
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
Codecov ReportAttention: Patch coverage is
✅ All tests successful. No failed tests found.
❌ Your patch check has failed because the patch coverage (98.71%) is below the target coverage (100.00%). You can increase the patch coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## master #831 +/- ##
==========================================
+ Coverage 96.38% 97.05% +0.67%
==========================================
Files 10 10
Lines 249 272 +23
Branches 7 6 -1
==========================================
+ Hits 240 264 +24
+ Misses 9 8 -1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
| "D213", # multi-line-summary-second-line. Incompatible with multi-line-summary-first-line (D212) | ||
| "D203", # one-blank-line-before-class. Incompatible with no-blank-line-before-class (D211) |
There was a problem hiding this comment.
There's a convention setting per https://docs.astral.sh/ruff/settings/#lint_pydocstyle_convention. I think if you add the following, it'll automatically disable the incompatible rules:
[lint.pydocstyle]
convention = "pep257"| "CPY001", # Missing copyright notice at top of file | ||
| "D213", # multi-line-summary-second-line. Incompatible with multi-line-summary-first-line (D212) | ||
| "D203", # one-blank-line-before-class. Incompatible with no-blank-line-before-class (D211) | ||
| "INP001", # We use namespace packages in this project |
There was a problem hiding this comment.
Have you tried setting the config option instead? https://docs.astral.sh/ruff/settings/#namespace-packages
namespace-packages = ["src/pre_commit_terraform/"]| @@ -65,35 +64,37 @@ def invoke_cli_app(self, parsed_cli_args: Namespace) -> ReturnCodeType: | |||
| [CustomCmdStub()], | |||
| ) | |||
|
|
|||
| assert ReturnCode.ERROR == invoke_cli_app(['sentinel']) | |||
There was a problem hiding this comment.
lol. Didn't know that ruff able to replace code in this way
| self, subcommand_parser: ArgumentParser, | ||
| ) -> None: | ||
| @staticmethod | ||
| def populate_argument_parser(subcommand_parser: ArgumentParser) -> None: # noqa: ARG004 |
There was a problem hiding this comment.
There was a problem hiding this comment.
This would need a justification comment, then.
But wait, a second… Why are you even changing this to a @staticmethod? You shouldn't do that. This is a stub for the interface that exists elsewhere, the signatures must match exactly.
| @@ -24,8 +25,7 @@ def test_arg_parser_populated() -> None: | |||
| def test_check_is_deprecated() -> None: | |||
| """Verify that `replace-docs` shows a deprecation warning.""" | |||
| deprecation_msg_regex = ( | |||
| r'^`terraform_docs_replace` hook is DEPRECATED\.' | |||
| 'For migration.*$' | |||
| r'^`terraform_docs_replace` hook is DEPRECATED\.' + 'For migration.*$' | |||
There was a problem hiding this comment.
don't concatenate static strings in runtime. we talked about this already..
|
|
||
| check_call_mock.assert_called_once_with(expected_cmd, shell=True) | ||
| assert invoke_cli_app(parsed_cli_args) == ReturnCode.ERROR | ||
| # We call cli tools, of course we use shell=True |
There was a problem hiding this comment.
Which is a terrible, dangerous idea and should be avoided instead of ignored…
There was a problem hiding this comment.
That pre-commit hooks which wrap external CLI tools. They are wrappers. How do you recommend to make wrappers to simplify tools execution and add some extra functional on top of tools, w/o calling underling tool?
There was a problem hiding this comment.
Call them without the dangerous shell=True argument.
unrelated is a separate PR. blame ignore is a separate PR. Plus I noticed reshuffling logic which is another one. So it's like 6 PRs. |
|
Also, plan for adding https://wemake-python-styleguide.rtfd.io on top after all this. |
| from subprocess import CalledProcessError | ||
| from argparse import ArgumentParser | ||
| from argparse import Namespace | ||
| from subprocess import CalledProcessError # noqa: S404. We invoke cli tools |
There was a problem hiding this comment.
Subprocess interaction should probably be quarantined in a dedicated module where these noqas would be allowed and nowhere else. That module should implement wrappers that the rest of the project would call. This design allows keeping all potentially dangerous code in one place that you can be extra careful about.
TBD:
After @webknjaz will approve config, split to 4 PRs:
git blameto s.2