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 1 commit
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.
Since more places will use subprocesses, it might be a good idea to have a helper module that imports subprocess and wraps it into another function. Then use that helper everywhere. This
noqa
would be in that one centralized location. This is a security-related linter (coming from the bandit plugin) so it's a good idea to bundle such violations in a single location with the justification documented and easily inspectable.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 have a slightly related question: Can we somehow utilize pip-installable terraform binary?
https://github.com/AleksaC/terraform-py
To be able to utilize https://pre-commit.com/#config-additional_dependencies or something like that, that will make users able to choose terraform version right into hooks configuration
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.
sys.executable
holds the absolute path to the current Python runtime binary. Things installed into the same virtualenv will be in the same directory if they ship scripts.So doing something like
str(pathlib.Path(sys.executable).parent.resolve() / 'terraform-py')
will allow you to run that other binary.Since that project also has a runpy interface (via the
__main__.py
), instead of invokingterraform-py
, you can invokepython -Im terraform_py
instead.When running it from here, you can construct the command for passing into
subprocess
' functions smth like(sys.executable, '-I', '-m', 'terraform_py')
.