refactor: Use inheritance to construct plugin CLI

[edgarrmondragon]

No description provided.

[codecov]

Codecov Report

Merging #936 (a8818b6) into main (e46a5e0) will increase coverage by 0.34%.
The diff coverage is 56.04%.

@@            Coverage Diff             @@
##             main     #936      +/-   ##
==========================================
+ Coverage   85.13%   85.47%   +0.34%     
==========================================
  Files          59       59              
  Lines        4890     4888       -2     
  Branches      810      802       -8     
==========================================
+ Hits         4163     4178      +15     
+ Misses        527      510      -17     
  Partials      200      200              

Impacted Files	Coverage Δ
singer_sdk/helpers/_util.py	50.00% <ø> (ø)
singer_sdk/tap_base.py	73.97% <26.47%> (+2.93%)	⬆️
singer_sdk/mapper_base.py	71.42% <30.76%> (+17.72%)	⬆️
singer_sdk/plugin_base.py	83.92% <68.42%> (+0.05%)	⬆️
singer_sdk/cli/__init__.py	100.00% <100.00%> (ø)
singer_sdk/configuration/_dict_config.py	100.00% <100.00%> (ø)
singer_sdk/target_base.py	89.25% <100.00%> (+2.19%)	⬆️

... and 1 file with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

[kgpayne]

@edgarrmondragon have you seen this?

I was wondering if a similar approach would allow us to add a new attribute cli_class to the Tap/Target base classes, which users could override with their own subclasses of the default Cli class, and return self.cli_class().run() from the existing cli attribute, rather than a Click command 🤔 I really like how the approach allows Click commands to be defined as class attributes 😍

[edgarrmondragon]

@edgarrmondragon have you seen this?

Thanks for sharing @kgpayne! After reviewing it, I don't think a pattern like that is something we can adopt without significant changes to the plugin class. A lot of the CLI options we currently support can and should be handled by simple callbacks (as this PR implements) and following the pattern in the link prohibits setting callbacks from class methods for things like version, discovery, etc. It also forces us to implement a God-method that handles all options (the status quo), which gets ugly as exemplified by tap-airbyte's implementation requiring too many copy and pasting.

I was wondering if a similar approach would allow us to add a new attribute cli_class to the Tap/Target base classes, which users could override with their own subclasses of the default Cli class, and return self.cli_class().run() from the existing cli attribute, rather than a Click command 🤔 I really like how the approach allows Click commands to be defined as class attributes 😍

I've pushed another commit to the PR that changes the PluginCLI descriptor to be a decorator. It enables uses like:

class ShortcutTap(Tap):
    @plugin_cli
    def update_schema(cls) -> click.Command:
        """Update the OpenAPI schema for this tap."""
        @click.command()
        def update():
            response = requests.get(
                "https://developer.shortcut.com/api/rest/v3/shortcut.swagger.json",
                timeout=5,
            )
            with Path("tap_shortcut/openapi.json").open("w") as f:
                f.write(response.text)

        return update

Also also, there's nothing stopping users from declaring custom CLIs and registering in [tool.poetry.scripts].

Wdyt?

[kgpayne]

@edgarrmondragon this looks great 🙌 Will do a full review tomorrow, but a docs addition would be helpful to support adoption/experimentation e.g. MeltanoLabs/target-snowflake#21 🙂

[edgarrmondragon]

@edgarrmondragon this looks great 🙌 Will do a full review tomorrow, but a docs addition would be helpful to support adoption/experimentation e.g. MeltanoLabs/target-snowflake#21 🙂

Thanks for calling out the lack of docs! I've added a short guide in https://meltano-sdk--936.org.readthedocs.build/en/936/guides/custom-clis.html

[kgpayne]

@edgarrmondragon my only thought is that it'd be great to have a more full-featured example for the docs in time. I learned from the Click codebase that click.Group is a subclasses of click.Command (via click.MultiCommand), so it would be possible to ship a multi-command CLI (along the lines of target-snowflake#21) in addition to the Singer-compatible CLI 🚀 So there may be future value in standardising on a <plugin-name>-utils or <plugin-name>-tools entrypoint (rather than an entrypoint/command per util), and maybe advertising the extra functionality as capabilities in --about. No action needed, just registering the thought to check I've understood the potential here. Happy to try out the pattern in target-snowflake before any future docs changes 🙂

[edgarrmondragon]

Happy to try out the pattern in target-snowflake before any future docs changes 🙂

Sounds good 👍