-
-
Notifications
You must be signed in to change notification settings - Fork 541
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: Backpush to main changes to skeleton of python hooks #741
base: master
Are you sure you want to change the base?
Changes from all commits
18291bd
9409f77
ef844ce
806a87a
3f3fdd9
a37971b
85269df
8fbba99
0421801
1f956dc
59b6bac
79ac415
93cddb7
0280a60
a14589a
b1a466c
06d4fc6
878f5bd
dae3af2
f99ca4a
c254a5b
0cdbaec
e5914e0
c033767
01a5850
c7c8e56
fe92d45
e010c8a
4c459df
33e1547
7d9ba2f
3a59fe4
7d96f1b
0c267ee
a3e4252
db3fefd
00674ef
b6aa20b
60f1292
7dde1df
52cf580
c03e3fe
aa71534
3285431
96e44ef
2c8b44b
934202f
3cbc220
7e647ad
0defc96
7b3cc6c
f1d8faf
2e54190
1f525fc
79b633e
294cf79
f1673ce
0e03e68
83c7a29
650d90c
8953dd9
53e2dd0
3cf99fd
89e5a45
c2aab92
449d448
ac42c51
a278a04
0799817
d3df63b
56ab01e
f622b10
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1 +1,4 @@ | ||
tests/results/* | ||
__pycache__/ | ||
*.py[cod] | ||
node_modules/ |
Original file line number | Diff line number | Diff line change | ||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -45,7 +45,7 @@ repos: | |||||||||||||||||
|
||||||||||||||||||
# Dockerfile linter | ||||||||||||||||||
- repo: https://github.com/hadolint/hadolint | ||||||||||||||||||
rev: v2.12.1-beta | ||||||||||||||||||
rev: v2.13.1-beta | ||||||||||||||||||
hooks: | ||||||||||||||||||
- id: hadolint | ||||||||||||||||||
args: [ | ||||||||||||||||||
|
@@ -66,3 +66,94 @@ repos: | |||||||||||||||||
- id: prettier | ||||||||||||||||||
# https://prettier.io/docs/en/options.html#parser | ||||||||||||||||||
files: '.json5$' | ||||||||||||||||||
|
||||||||||||||||||
|
||||||||||||||||||
########## | ||||||||||||||||||
# PYTHON # | ||||||||||||||||||
########## | ||||||||||||||||||
|
||||||||||||||||||
- repo: https://github.com/astral-sh/ruff-pre-commit | ||||||||||||||||||
rev: v0.8.4 | ||||||||||||||||||
hooks: | ||||||||||||||||||
- id: ruff | ||||||||||||||||||
args: [--fix] | ||||||||||||||||||
types_or: [python, pyi] | ||||||||||||||||||
- id: ruff-format | ||||||||||||||||||
types_or: [python, pyi] | ||||||||||||||||||
args: [--config, format.quote-style = 'single', --config, line-length = 100] | ||||||||||||||||||
|
||||||||||||||||||
- repo: https://github.com/pycqa/isort | ||||||||||||||||||
rev: 5.13.2 | ||||||||||||||||||
hooks: | ||||||||||||||||||
- id: isort | ||||||||||||||||||
name: isort | ||||||||||||||||||
args: [--force-single-line, --profile=black] | ||||||||||||||||||
exclude: | | ||||||||||||||||||
(?x) | ||||||||||||||||||
# Uses incorrect indentation in imports in places where it shouldn't | ||||||||||||||||||
# https://github.com/PyCQA/isort/issues/2315#issuecomment-2566703698 | ||||||||||||||||||
(^tests/pytest/test__cli_subcommands\.py$ | ||||||||||||||||||
) | ||||||||||||||||||
|
||||||||||||||||||
- repo: https://github.com/asottile/add-trailing-comma | ||||||||||||||||||
rev: v3.1.0 | ||||||||||||||||||
hooks: | ||||||||||||||||||
- id: add-trailing-comma | ||||||||||||||||||
|
||||||||||||||||||
- repo: https://github.com/pre-commit/mirrors-autopep8 | ||||||||||||||||||
rev: v2.0.4 | ||||||||||||||||||
hooks: | ||||||||||||||||||
- id: autopep8 | ||||||||||||||||||
args: | ||||||||||||||||||
- -i | ||||||||||||||||||
- --max-line-length=100 | ||||||||||||||||||
|
||||||||||||||||||
# Usage: http://pylint.pycqa.org/en/latest/user_guide/message-control.html | ||||||||||||||||||
- repo: https://github.com/PyCQA/pylint | ||||||||||||||||||
rev: v3.3.3 | ||||||||||||||||||
hooks: | ||||||||||||||||||
- id: pylint | ||||||||||||||||||
args: | ||||||||||||||||||
- --disable=import-error # E0401. Locally you could not have all imports. | ||||||||||||||||||
- --disable=fixme # W0511. 'TODO' notations. | ||||||||||||||||||
- --disable=logging-fstring-interpolation # Conflict with "use a single formatting" WPS323 | ||||||||||||||||||
- --disable=ungrouped-imports # ignore `if TYPE_CHECKING` case. Other do reorder-python-imports | ||||||||||||||||||
- --disable=R0801 # Similar lines in 2 files. Currently I don't think that it possible to DRY hooks unique files boilerplate | ||||||||||||||||||
|
||||||||||||||||||
exclude: test_.+\.py$ | ||||||||||||||||||
|
||||||||||||||||||
- repo: https://github.com/pre-commit/mirrors-mypy | ||||||||||||||||||
rev: v1.14.1 | ||||||||||||||||||
hooks: | ||||||||||||||||||
- id: mypy | ||||||||||||||||||
additional_dependencies: | ||||||||||||||||||
- types-PyYAML | ||||||||||||||||||
args: [ | ||||||||||||||||||
--ignore-missing-imports, | ||||||||||||||||||
--disallow-untyped-calls, | ||||||||||||||||||
--warn-redundant-casts, | ||||||||||||||||||
] | ||||||||||||||||||
|
||||||||||||||||||
- repo: https://github.com/wemake-services/wemake-python-styleguide | ||||||||||||||||||
rev: 1.0.0 | ||||||||||||||||||
hooks: | ||||||||||||||||||
- id: wemake-python-styleguide | ||||||||||||||||||
args: | ||||||||||||||||||
- --allowed-module-metadata=__all__ # Default to '' | ||||||||||||||||||
- --max-local-variables=6 # Default to 5 | ||||||||||||||||||
- --max-returns=6 # Default to 5 | ||||||||||||||||||
- --max-imports=15 # Default to 12 | ||||||||||||||||||
Comment on lines
+142
to
+145
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Don't accept this just yet. It shouldn't even be in this config. See my other comments. |
||||||||||||||||||
# https://wemake-python-stylegui.de/en/latest/pages/usage/violations/index.html | ||||||||||||||||||
- --extend-ignore= | ||||||||||||||||||
E501 <!-- line too long (> 79 characters). Use 100 --> | ||||||||||||||||||
WPS115 <!-- Found upper-case constant in a class. All occurrences are false positive --> | ||||||||||||||||||
WPS226 <!-- Found string literal over-use - append > 3 --> | ||||||||||||||||||
|
||||||||||||||||||
|
||||||||||||||||||
exclude: | | ||||||||||||||||||
(?x) | ||||||||||||||||||
# Ignore tests | ||||||||||||||||||
(^tests/pytest/test_.+\.py$ | ||||||||||||||||||
# Ignore deprecated hook | ||||||||||||||||||
|^src/pre_commit_terraform/terraform_docs_replace\.py$ | ||||||||||||||||||
) |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -37,7 +37,7 @@ | |
name: Terraform docs (overwrite README.md) | ||
description: Overwrite content of README.md with terraform-docs. | ||
require_serial: true | ||
entry: python -Im pre_commit_terraform replace-docs | ||
entry: python -Im pre_commit_terraform terraform_docs_replace | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Plz, stick to kebab-case CLI arguments. It's rather uncommon to use snake_case in CLIs. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. +1 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I want to maintain one name across everything to not create more entropy. If hook has named There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That's the thing — the only place for the hook subcommand name is the constant in the module, not the module itself. Here's some discussion in other popular CLI frameworks: fastapi/typer#341 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Here's some public arguments discussing the ergonomics of having to use Shift-key vs. not, leaning towards kebab-space or no-delimiter:
Do you have many programs on your system that have |
||
language: python | ||
files: (\.tf)$ | ||
exclude: \.terraform/.*$ | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -7,4 +7,7 @@ | |
"code-block-style": false | ||
}, | ||
"markdown.validate.enabled": true, | ||
"python.analysis.extraPaths": [ | ||
"./src" | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. should also check the tests dirs |
||
], | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,5 +1,6 @@ | ||
[build.targets.sdist] | ||
include = [ | ||
'.pre-commit-hooks.yaml', | ||
'src/', | ||
] | ||
|
||
|
@@ -8,6 +9,9 @@ packages = [ | |
'src/pre_commit_terraform/', | ||
] | ||
|
||
[build.targets.wheel.force-include] | ||
'.pre-commit-hooks.yaml' = 'pre_commit_terraform/_artifacts/.pre-commit-hooks.yaml' | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This should probably just be picked into master directly. |
||
|
||
[metadata.hooks.vcs.urls] | ||
'Source Archive' = 'https://github.com/antonbabenko/pre-commit-terraform/archive/{commit_hash}.tar.gz' | ||
'GitHub: repo' = 'https://github.com/antonbabenko/pre-commit-terraform' | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -9,16 +9,15 @@ build-backend = 'hatchling.build' | |
name = 'pre-commit-terraform' | ||
classifiers = [ | ||
'License :: OSI Approved :: MIT License', | ||
'Programming Language :: Python :: 2', | ||
'Programming Language :: Python :: 2.7', | ||
'Programming Language :: Python :: 3', | ||
'Programming Language :: Python :: 3.6', | ||
'Programming Language :: Python :: 3.7', | ||
'Programming Language :: Python :: 3 :: Only', | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's common to list which environments it's being tested under. And additionally, the most important bit in the metadata would be the |
||
'Programming Language :: Python :: Implementation :: CPython', | ||
'Programming Language :: Python :: Implementation :: PyPy', | ||
] | ||
description = 'Pre-commit hooks for terraform_docs_replace' | ||
dependencies = [] | ||
description='Pre-commit hooks for terraform' | ||
dependencies = [ | ||
'pyyaml', | ||
] | ||
dynamic = [ | ||
'urls', | ||
'version', | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,5 @@ | ||
[pytest] | ||
pythonpath = src | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is not needed. And also, I've got a proper config in #742 anyway. |
||
|
||
testpaths = | ||
tests/pytest |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,9 +1,9 @@ | ||
"""A runpy-style CLI entry-point module.""" | ||
|
||
from sys import argv, exit as exit_with_return_code | ||
|
||
from ._cli import invoke_cli_app | ||
from sys import argv | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This will conflict with a fix in #742 FYI (this specific import is being dropped). |
||
from sys import exit as exit_with_return_code | ||
|
||
from pre_commit_terraform._cli import invoke_cli_app | ||
|
||
return_code = invoke_cli_app(argv[1:]) | ||
exit_with_return_code(return_code) |
Original file line number | Diff line number | Diff line change | ||||||||
---|---|---|---|---|---|---|---|---|---|---|
|
@@ -6,7 +6,50 @@ | |||||||||
|
||||||||||
from argparse import ArgumentParser | ||||||||||
|
||||||||||
from ._cli_subcommands import SUBCOMMAND_MODULES | ||||||||||
from pre_commit_terraform._cli_subcommands import SUBCOMMAND_MODULES | ||||||||||
|
||||||||||
|
||||||||||
def populate_common_argument_parser(parser: ArgumentParser) -> None: | ||||||||||
""" | ||||||||||
Populate the argument parser with the common arguments. | ||||||||||
|
||||||||||
Args: | ||||||||||
parser (argparse.ArgumentParser): The argument parser to populate. | ||||||||||
Comment on lines
+16
to
+17
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This API doc style is non-native to Sphinx. You might have to specify it in Here's the native syntax FYI: https://www.sphinx-doc.org/en/master/usage/domains/python.html#info-field-lists. I haven't checked if WPS replaced darglint w/ something else, it might not be as bad. Also, with some Sphinx extensions, there shouldn't be a need to duplicate the argument types in docstrings. Plus, since you don't have Sphinx, this is definitely not something you should be spending additional time on. Just describe the args and the return value, omitting the types as it's not worth it. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Interesting, what Google use then for docs, if that's not suported by Sphinx There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's a custom style that's not enabled by default. It's implemented by the napoleon extension: https://sphinxcontrib-napoleon.rtfd.io / https://www.sphinx-doc.org/en/master/usage/extensions/napoleon.html. |
||||||||||
""" | ||||||||||
parser.add_argument( | ||||||||||
'-a', | ||||||||||
'--args', | ||||||||||
action='append', | ||||||||||
help='Arguments that configure wrapped tool behavior', | ||||||||||
default=[], | ||||||||||
) | ||||||||||
parser.add_argument( | ||||||||||
'-h', | ||||||||||
'--hook-config', | ||||||||||
action='append', | ||||||||||
metavar='KEY=VALUE', | ||||||||||
help='Arguments that configure hook behavior', | ||||||||||
default=[], | ||||||||||
) | ||||||||||
parser.add_argument( | ||||||||||
'-i', | ||||||||||
'--tf-init-args', | ||||||||||
'--init-args', | ||||||||||
action='append', | ||||||||||
help='Arguments for `tf init` command', | ||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There could be not only There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Then let's abstract the name, like f.e.:
Suggested change
|
||||||||||
default=[], | ||||||||||
) | ||||||||||
parser.add_argument( | ||||||||||
'-e', | ||||||||||
'--env-vars', | ||||||||||
'--envs', | ||||||||||
dest='env_vars_strs', | ||||||||||
metavar='KEY=VALUE', | ||||||||||
action='append', | ||||||||||
help='Setup additional Environment Variables during hook execution', | ||||||||||
default=[], | ||||||||||
) | ||||||||||
parser.add_argument('files', nargs='*', help='Changed files paths') | ||||||||||
|
||||||||||
|
||||||||||
def attach_subcommand_parsers_to(root_cli_parser: ArgumentParser, /) -> None: | ||||||||||
|
@@ -18,19 +61,27 @@ def attach_subcommand_parsers_to(root_cli_parser: ArgumentParser, /) -> None: | |||||||||
""" | ||||||||||
subcommand_parsers = root_cli_parser.add_subparsers( | ||||||||||
dest='check_name', | ||||||||||
help='A check to be performed.', | ||||||||||
required=True, | ||||||||||
) | ||||||||||
for subcommand_module in SUBCOMMAND_MODULES: | ||||||||||
subcommand_parser = subcommand_parsers.add_parser(subcommand_module.CLI_SUBCOMMAND_NAME) | ||||||||||
subcommand_parser = subcommand_parsers.add_parser( | ||||||||||
subcommand_module.HOOK_ID, | ||||||||||
add_help=False, | ||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I still maintain that it'd be useful to allow There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Agree, but that's out of scope for now - firstly we need to reimplement same hooks in Python w/o any breaking changes about what we are aware. Anyway, I added that as TODO in |
||||||||||
) | ||||||||||
subcommand_parser.set_defaults( | ||||||||||
invoke_cli_app=subcommand_module.invoke_cli_app, | ||||||||||
) | ||||||||||
subcommand_module.populate_argument_parser(subcommand_parser) | ||||||||||
populate_common_argument_parser(subcommand_parser) | ||||||||||
subcommand_module.populate_hook_specific_argument_parser(subcommand_parser) | ||||||||||
|
||||||||||
|
||||||||||
def initialize_argument_parser() -> ArgumentParser: | ||||||||||
"""Return the root argument parser with sub-commands.""" | ||||||||||
""" | ||||||||||
Parse the command line arguments and return the parsed arguments. | ||||||||||
|
||||||||||
Return the root argument parser with sub-commands. | ||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Only the docstring title needs to be in imperative mood. The long description paragraph doesn't need to do that, it can be more explanatory. |
||||||||||
|
||||||||||
""" | ||||||||||
root_cli_parser = ArgumentParser(prog=f'python -m {__package__ !s}') | ||||||||||
attach_subcommand_parsers_to(root_cli_parser) | ||||||||||
return root_cli_parser | ||||||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,12 +1,9 @@ | ||
"""A CLI sub-commands organization module.""" | ||
|
||
from . import terraform_docs_replace | ||
from ._types import CLISubcommandModuleProtocol | ||
from pre_commit_terraform import terraform_docs_replace | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. FTR #742 has proper configuration that can work with relative imports just fine. |
||
from pre_commit_terraform._types import CLISubcommandModuleProtocol | ||
|
||
|
||
SUBCOMMAND_MODULES: list[CLISubcommandModuleProtocol] = [ | ||
terraform_docs_replace, | ||
] | ||
SUBCOMMAND_MODULES: tuple[CLISubcommandModuleProtocol, ...] = (terraform_docs_replace,) | ||
|
||
|
||
__all__ = ('SUBCOMMAND_MODULES',) |
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.
pre-commit-terraform
repo I guessargs: […]
) or multiline (as in YAML sequence) and definitely w/o single-line arrays split to multiline please.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 also have this sort of comments in a draft review that I haven't yet posted in #743.
On the CLI settings, they should almost never be used not because of the defaults but because any other calls to those tools (like IDE extensions automatically picking them up w/o the user configuring anything) would not consume these settings and will result in different sets of violations reported and different behaviors of tools that aren't linters.
And for consistency, I'd rather enforce
yamllint
. I can send in my default config separately.Additionally, the organization of integrations in this config is suboptimal that I'll expand on in the other review.