Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
chore(linters): Introduce ruff and fix issues #831
base: master
Are you sure you want to change the base?
chore(linters): Introduce ruff and fix issues #831
Changes from 14 commits
7e1dd4f
9430c1a
8dc8451
8c3d385
53bb73f
8831b60
665d46c
87273be
fa302b0
f74ef59
e3c1272
71e794e
b309d69
948a36d
7765818
51d9a93
bd24ee0
8384a66
460031a
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
This is not the built-in Sphinx API doc style. Use
:returns:
instead. And:rtype:
is probably unnecessary here as the annotation exists.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.
https://docs.astral.sh/ruff/rules/docstring-missing-returns/
Details
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.
Huh?
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.
Naming. It wants
Error
suffix in name as it based on PreCommitTerraformBaseErrorThere 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.
Why does it say FIXME then? Add justification to the suppression. The name mimics the built-in
SystemExit
and is meant to have exactly the same semantics. For this reason, it shouldn't haveError
in the name to maintain resemblance.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.
https://docs.astral.sh/ruff/rules/error-suffix-on-exception-name/
Because rule reference to PEP8, and it was logical to leave
FIXME
for some future investigation.So you want just suppress this rule with comment
The name mimics the built-in SystemExit and is meant to have exactly the same semantics. For this reason, it shouldn't have Error in the name to maintain resemblance.
?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.
Yes, suppress it and leave a justification comment. It shouldn't be changed because it's an intentional special case.
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.
Why is this commented? Have you tried uncommenting?
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.
No idea.
Will try
UPD. Mar 25th: I have broken pytest in tox locally, no idea why. Can't test now, will return to it on next iteration
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 breaks tests
Details
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.
Okay,
test_app_exit
actually covers this case because it's supposed to work a bit differently.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.
Anything against args shifted with a different indent than the function body?
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.
Nothing from my side. Need to find ruff config for that then
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's autofixed by ruff even if there no
ruff.toml
Maybe that's because it has
)
without indents?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.
No, most tools use this formatting. But some have rules for extra indent. I like the extra indent style because then, it's not aligned with the following function body and is visually distinct.
You can change the style. Just make sure to do so in a separate PR.
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.
Urgh..
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.
lol. Didn't know that ruff able to replace code in this way
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.
Yep, some of the Ruff's linting rules also support autofixing feature. This is what happens when you pass
--fix
. It's fine, just keep it separate.