chore(linters): Apply ruff linter semi-automatic linting fixes#863
chore(linters): Apply ruff linter semi-automatic linting fixes#863MaxymVlasov merged 12 commits intomasterfrom
Conversation
…rocArgs.append()`
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe changes modify several CLI-related modules and their tests. In the core modules, import statements are split for clarity, and type casting is adjusted to use string representations rather than direct type references. Minor formatting improvements, such as adding trailing commas, have been made. Test files now include updated error handling, assertion syntax, and revised exception raising. Overall, the modifications refine the code style and consistency across the CLI invocation functions and their tests without altering functionality. Changes
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Note 🎁 Summarized by CodeRabbit FreeYour organization has reached its limit of developer seats under the Pro Plan. For new users, CodeRabbit will generate a high-level summary and a walkthrough for each pull request. For a comprehensive line-by-line review, please add seats to your subscription by visiting https://app.coderabbit.ai/login.If you believe this is a mistake and have available seats, please assign one to the pull request author through the subscription management page using the link above. 🪧 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
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
src/pre_commit_terraform/__main__.py(1 hunks)src/pre_commit_terraform/_cli.py(1 hunks)src/pre_commit_terraform/_cli_parsing.py(1 hunks)src/pre_commit_terraform/terraform_docs_replace.py(2 hunks)tests/pytest/_cli_test.py(3 hunks)tests/pytest/terraform_docs_replace_test.py(3 hunks)
🧰 Additional context used
🧬 Code Definitions (1)
tests/pytest/terraform_docs_replace_test.py (4)
tests/pytest/_cli_test.py (4)
invoke_cli_app(61-62)invoke_cli_app(91-93)populate_argument_parser(55-59)populate_argument_parser(85-89)src/pre_commit_terraform/terraform_docs_replace.py (2)
invoke_cli_app(49-89)populate_argument_parser(14-46)src/pre_commit_terraform/_types.py (2)
invoke_cli_app(26-28)populate_argument_parser(20-24)src/pre_commit_terraform/_cli.py (1)
invoke_cli_app(16-53)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: 🧹 Linters / pre-commit@🐍3.13@ubuntu-latest
- GitHub Check: pre-commit
🔇 Additional comments (15)
src/pre_commit_terraform/_cli_parsing.py (1)
26-26: Trailing comma addedAdding a trailing comma in multi-line function calls is a good practice that improves version control diffs and makes it easier to add parameters in the future.
src/pre_commit_terraform/__main__.py (1)
3-4: Split import statements for better readabilitySplitting the import of
argvandexitinto separate lines follows Python best practices of one import per line, making the code more maintainable and readable.src/pre_commit_terraform/_cli.py (1)
26-26:Details
✅ Verification successful
String literal for type casting
Using a string literal ('CLIAppEntryPointCallableType') instead of direct type reference is aligned with the FIXME comment above that references a Stack Overflow solution for typing issues. This approach helps avoid potential circular import issues in typing.
🏁 Script executed:
#!/bin/bash # Check if this pattern is consistently applied across the codebase echo "Checking for other instances of cast_to in the codebase..." rg "cast_to\(" --type py -A 1 -B 1Length of output: 1327
Consistent String Literal Usage for Type Casting is Verified
The use of the string literal
'CLIAppEntryPointCallableType'for type casting insrc/pre_commit_terraform/_cli.pyis confirmed to be consistent with other instances (e.g., insrc/pre_commit_terraform/terraform_docs_replace.py). This approach, aligned with the referenced FIXMEs and Stack Overflow solution, effectively avoids circular import issues in typing. No further changes are required.tests/pytest/terraform_docs_replace_test.py (4)
8-9: Standalone pytest importMoving pytest to its own import line follows Python styling conventions of one import per line.
13-16: Separate import blocks for better organizationSplitting the imports from the same module into separate blocks enhances readability and organization. This is a good practice that helps visualize different functional groups of imports.
87-87: Assertion order improvementChanged the assertion order to follow the conventional pattern of
assert actual == expectedrather thanassert expected == actual. This improves readability and matches common testing conventions.
122-122: Assertion order improvementSame improvement as above - changed to the conventional
assert actual == expectedpattern, which provides more intuitive error messages when tests fail.src/pre_commit_terraform/terraform_docs_replace.py (4)
56-56: Good addition of stacklevel parameter to warningAdding
stacklevel=2to the warning is a proper practice that will ensure the warning points to the caller's code rather than this function. This helps users identify where in their code the deprecated hook is being used.
60-60: Improved type annotation formatChanging from direct type reference
cast_to(list[str], ...)to string representationcast_to('list[str]', ...)aligns with modern Python practices for postponed evaluation of annotations (PEP 563). This is a good update.
72-72: Improved type annotation formatSimilar to the previous change, using string representation for type annotations is the preferred approach in modern Python.
74-84: Code refactoring for cleaner argument buildingUsing
procArgs.extend()with a tuple is a cleaner approach than multiple append calls. This improves code maintainability.tests/pytest/_cli_test.py (4)
5-5: Import format updateThe import formatting for pytest has been standardized, which improves code consistency.
10-10: Restored necessary importGood restoration of
PreCommitTerraformExitimport, which is used in the test_app_exit function.
70-70: Improved assertion syntaxThe assertion has been updated to use a more explicit equality check, which better conveys the intent of the test.
92-93: Enhanced readability with variable introductionIntroducing a variable
msgbefore raising the exception improves readability. It's also a more maintainable approach if the message needs to be reused or built from multiple parts in the future.
| '>', | ||
| './{dir}/{dest}'.format( | ||
| dir=dir, | ||
| dest=cast_to('bool', parsed_cli_args.dest), |
There was a problem hiding this comment.
Fix incorrect type casting
There's a bug in this line. You're casting parsed_cli_args.dest to 'bool' when it should be a string (default is 'README.md' as defined in line 23).
- dest=cast_to('bool', parsed_cli_args.dest),
+ dest=cast_to('str', parsed_cli_args.dest),📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| dest=cast_to('bool', parsed_cli_args.dest), | |
| dest=cast_to('str', parsed_cli_args.dest), |
There was a problem hiding this comment.
Can we have this rectified please? I mean accepting coderabbit's code suggestion?
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
There was a problem hiding this comment.
Can we have this rectified please? I mean accepting coderabbit's code suggestion?
FWIW, this is not really related to the linting violations that are being fixed in this PR. It might be better to do this separately.
There was a problem hiding this comment.
Your organization has reached its limit of developer seats under the Pro Plan. In order to use the Chat feature, you need to buy additional seats
There was a problem hiding this comment.
FWIW, this is not really related to the linting violations that are being fixed in this PR. It might be better to do this separately.
Let's do it one way or another. Coderabbit is crying and I also don't think the confusing expression is worth of keeping in code once we've found it.
There was a problem hiding this comment.
Your organization has reached its limit of developer seats under the Pro Plan. In order to use the Chat feature, you need to buy additional seats
|
I guess @webknjaz suggests (even insists) we make no manual changes to such PRs as their merge commits are meant to go into blame ignore file, right? |
|
@yermulnik no, these changes must not go into blame ignore at all. It's not formatting. The fact that these changes were generated by ruff does not mean that they are any less of linting fixes with runtime impact more often than not. Such patches shouldn't be hidden. Only real mass formatting should be. |
FWIW, once in the f-string, that entire |
|
In general, it's okay to add fixes here that relate directly to fixing the rule violations. Especially, since one 55009e6 broke the CI and mandates an additional change in the tests to make it work. |
MaxymVlasov
changed the title
Co-authored-by: 🇺🇦 Sviatoslav Sydorenko (Святослав Сидоренко) <webknjaz@redhat.com>
MaxymVlasov
changed the title
Codecov ReportAttention: Patch coverage is
❌ Your patch check has failed because the patch coverage (95.23%) 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 #863 +/- ##
=======================================
Coverage 98.44% 98.44%
=======================================
Files 28 28
Lines 578 578
Branches 17 17
=======================================
Hits 569 569
Misses 9 9
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
Ah, okay, I've gone lost in the amount of these auto-semi-fixes =)
@MaxymVlasov Let's get this fixed then.
And meanwhile it's for sure isn't worthwhile to keep that incorrect casting I guess as it introduces confusion of why casting string to boolean. |
Already fixed |
It doesn't introduce the problem as a part of the linting fixes. It's pre-existing and would be better to address separately. Making additional changes to an arbitrary number of places that fall out of the narrow scope of this PR will cause the same problems we're seeing in #831. Let's not contribute to unnecessary merge delays. |
For me it's still there 😕 |
I'm ok to approve and will do it now, though the delay is because we can't agree on the most greatest way to fix this single bit of obviously wrong code bit 🤷🏻 Get this fixed and, while you both are on it, you'll catch if any part of the code relies upon this incorrect bit of code (as opposite to postponing this to a separate PR that who knows whether we won't miss to create). |
It's not the same thing you're quoting. |
That cast only influences MyPy runs. It exists to make MyPy interpret things as certain types, as a hack. It doesn't have any effect in runtime whatsoever. |
I'm not following you. My point is about that odd cast from string to bool. I'm talking about it and only it. So I just can't be quoting something else 🤷🏻
Honestly, I don't really care what that cast is for. It is confusing and I'd like this fixed. |
|
@yermulnik please create follow-up PR that will fix this cast. Tnx |
|
|
This PR is included in version 1.98.1 🎉 |
4 participants
Description of your changes
Subset of #831
Apply next for each violation:
and commit as separate commit
with same
ruff.tomlas in #831, except 2 last lines which enable ignore of deprecated hooks (https://github.com/antonbabenko/pre-commit-terraform/blob/948a36d52d36270c329028bd27f7a3e999c899ff/ruff.toml)