chore(refactoring): Introduce a Python CLI app layout by webknjaz · Pull Request #738 · antonbabenko/pre-commit-terraform

webknjaz · 2024-12-30T05:42:01Z

This includes a structure with purpose-based modules and a standard mechanism for adding more subcommands.

When adding a new subcommand, one has to define the invoke_cli_app() and populate_argument_parser() hooks in their new module together with a CLI_SUBCOMMAND_NAME string constant. I can then be wired into the framework via _cli_subcommands.py. This is the only integration point necessary.

populate_argument_parser() accepts a subparser instance of argparse.ArgumentParser() that a new subcommand would need to attach new arguments into. It does not need to return anything. And the invoke_cli_app() hook is called with an instance of argparse.Namespace() with all the arguments parsed and pre-processed. This function is supposed to have the main check logic and return an instance of ._structs.ReturnCode() or int.
CLI_SUBCOMMAND_NAME needs to be annotated as a Final[str].

pre-commit integration is done through the runpy interface: python -m pre_commit_terraform <subcommand>.

The import package folder also contains a more detailed maintenance manual: https://github.com/antonbabenko/pre-commit-terraform/tree/8e77580805322e5d264a2667b4f4cba53e8bf94c/src/pre_commit_terraform#readme.

Put an x into the box if that apply:

This PR introduces breaking change.
This PR fixes a bug.
This PR adds new functionality.
This PR enhances existing functionality.
This PR is an internal restructuring change.

Description of your changes

I think the most important details are in the commit messages. Everything that is unlikely to change much over time is in "_private" modules.

How can we test changes

Something like python -Im pip install -e . && python -Im pre_commit_terraform replace-docs. Or invoke it via pre-commit.

yermulnik · 2024-12-30T13:11:57Z

This includes a structure with purpose-based modules and a standard mechanism for adding more subcommands.

Cool. Thanks. Too much of Python'ish stuff for me 🤣

When adding a new subcommand, one has to wire the invoke_cli_app() and populate_argument_parser() from their new module into the mappings defined in _cli_subcommands.py and _cli_parsing.py respectively. This is the only integration point necessary.

populate_argument_parser() accepts a subparser instance of argparse.ArgumentParser() that a new subcommand would need to attach new arguments into. It does not need to return anything. And the invoke_cli_app() hook is called with an instance of argparse.Namespace() with all the arguments parsed and pre-processed. This function is supposed to have the main check logic and return an instance of ._structs.ReturnCode() or int.

Description of your changes

I think the most important details are in the commit message. Everything that is unlikely to change much over time is in "_private" modules.

How can we test changes

Something like python -Im pip install -e . && python -Im pre_commit_terraform replace-docs. Or invoke it via pre-commit.

Could you please drop a README.md file with the above info into the directory with these files for future reference? Thank you.

MaxymVlasov

Why there so much boilerplate -_-

MaxymVlasov · 2024-12-30T14:04:48Z

src/pre_commit_terraform/_cli_parsing.py

+from .terraform_docs_replace import (
+    populate_argument_parser as populate_replace_docs_argument_parser,
+)
+
+
+PARSER_MAP = {
+    'replace-docs': populate_replace_docs_argument_parser,
+}


So I will need to add here all python hooks?

And how it should work with common.py?

I don't understand how I will use it with

pre-commit-terraform/hooks/terraform_fmt.py

Line 29 in 33e1547

args, hook_config, files, _tf_init_args, env_vars_strs = common.parse_cmdline(argv)

Yes, this mapping is for parsers, and another file has a mapping for callables with the logic.

I don't understand how I will use it with

You won't need to call common.parse_cmdline(argv) manually in several places anymore. It'll be called for you, and you'll get the result of a parse_args() invocation into a function you'll define to accept an argparse.Namespace instance.

src/pre_commit_terraform/_structs.py

MaxymVlasov · 2024-12-30T14:26:29Z

Why you used terraform_docs_replace.py at all? This crap does not represent anything how current hooks works.

Sorry, but I have no idea how to reuse half of code implemented here with actual hooks

webknjaz · 2024-12-30T18:23:24Z

Why you used terraform_docs_replace.py at all? This crap does not represent anything how current hooks works.

Because it is a working example of implementing custom check subcommands until you add new ones. You can swap it out for new things later.

Sorry, but I have no idea how to reuse half of code implemented here with actual hooks

Hence, the example integration of the only existing thing. I'll drop the instructions into a readme as suggested.

webknjaz · 2024-12-30T19:22:44Z

I've added a document. While doing that, I realized that I can simplify the new subcommand integration a bit, so I've marked this as a draft until I do that.

yermulnik · 2024-12-30T19:32:31Z

I've added a document.

Pretty nice and clear ❤️

webknjaz · 2024-12-30T20:06:09Z

@yermulnik @MaxymVlasov this is now mergeable, but #734 should go in first to unblock the CI.

webknjaz · 2024-12-30T20:14:48Z

pre-commit.ci run

yermulnik

LGTM. Though since @MaxymVlasov is mainly the only one who develops code at this moment I'm deferring final decision to him.

This includes a structure with purpose-based modules and a standard mechanism for adding more subcommands. When adding a new subcommand, one has to wire the `invoke_cli_app()` and `populate_argument_parser()` from their new module into the mappings defined in `_cli_subcommands.py` and `_cli_parsing.py` respectively. This is the only integration point necessary. `populate_argument_parser()` accepts a subparser instance of `argparse.ArgumentParser()` that a new subcommand would need to attach new arguments into. It does not need to return anything. And the `invoke_cli_app()` hook is called with an instance of `argparse.Namespace()` with all the arguments parsed and pre-processed. This function is supposed to have the main check logic and return an instance of `._structs.ReturnCode()` or `int`.

Previously, adding a new subcommand required importing it into two separate Python modules, adding them into two mappings, maintaining the same dictionary key string. This is prone to human error and is underintegrated. This patch makes it easier by defining a required subcommand module shape on the typing level. Now, one just need to implement two hook- functions and a constant with specific signatures and import it in a single place.

See https://docs.python.org/3/library/__main__.html#id1.

src/pre_commit_terraform/_cli_parsing.py

MaxymVlasov

Let's hope that I'll understand all later :)

IT somehow works with new hooks, but need some polishing, which will be done in other PRs

antonbabenko · 2025-01-16T16:12:33Z

This PR is included in version 1.97.0 🎉

webknjaz requested review from MaxymVlasov and yermulnik as code owners December 30, 2024 05:42

webknjaz changed the title ~~💅 Introduce a Python CLI app layout~~ chore(refactoring): X💅 Introduce a Python CLI app layout Dec 30, 2024

MaxymVlasov reviewed Dec 30, 2024

View reviewed changes

webknjaz marked this pull request as draft December 30, 2024 18:42

webknjaz marked this pull request as ready for review December 30, 2024 20:00

webknjaz force-pushed the maintenance/python-cli-structure branch from cdf5b6e to 3af52a9 Compare December 30, 2024 20:05

yermulnik reviewed Dec 30, 2024

View reviewed changes

webknjaz added 5 commits December 30, 2024 21:49

Add a maintainer's manual into the importable package dir

b6aa20b

󰑌 Drop the __main__ check per Python docs

7dde1df

See https://docs.python.org/3/library/__main__.html#id1.

📝 Extend the manual w/ subcommand guide

52cf580

webknjaz force-pushed the maintenance/python-cli-structure branch from 1c12549 to 52cf580 Compare December 30, 2024 20:49

MaxymVlasov changed the title ~~chore(refactoring): X💅 Introduce a Python CLI app layout~~ chore(refactoring): Introduce a Python CLI app layout Dec 30, 2024

MaxymVlasov reviewed Dec 30, 2024

View reviewed changes

src/pre_commit_terraform/_cli_parsing.py Outdated Show resolved Hide resolved

Apply suggestions from code review

89dd32e

MaxymVlasov approved these changes Dec 30, 2024

View reviewed changes

MaxymVlasov merged commit 993b854 into antonbabenko:master Dec 30, 2024
4 checks passed

Uh oh!

Comments

Conversation

webknjaz commented Dec 30, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description of your changes

How can we test changes

Uh oh!

yermulnik commented Dec 30, 2024

Description of your changes

How can we test changes

Uh oh!

MaxymVlasov left a comment

Choose a reason for hiding this comment

Uh oh!

MaxymVlasov Dec 30, 2024

Choose a reason for hiding this comment

Uh oh!

MaxymVlasov Dec 30, 2024

Choose a reason for hiding this comment

Uh oh!

webknjaz Dec 30, 2024

Choose a reason for hiding this comment

Uh oh!

webknjaz Dec 30, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

MaxymVlasov commented Dec 30, 2024

Uh oh!

webknjaz commented Dec 30, 2024

Uh oh!

webknjaz commented Dec 30, 2024

Uh oh!

yermulnik commented Dec 30, 2024

Uh oh!

webknjaz commented Dec 30, 2024

Uh oh!

webknjaz commented Dec 30, 2024

Uh oh!

yermulnik left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

MaxymVlasov left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

antonbabenko commented Jan 16, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

webknjaz commented Dec 30, 2024 •

edited

Loading