Skip to content
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

mild CLI cleanup #16560

Open
wants to merge 15 commits into
base: main
Choose a base branch
from
Open

mild CLI cleanup #16560

wants to merge 15 commits into from

Conversation

aaazzam
Copy link
Collaborator

@aaazzam aaazzam commented Dec 31, 2024

In the spirit of #15008 and #16292, this PR

  • adds an explicit acommand method to Prefect's Typer subclass to handle asynchronous CLI commands.
  • removes some method overrides that obfuscated typing and incorrectly forwarded kwargs to the relevant super().
    • In particular, removes our custom deprecation handling (which wasn't hooked up correctly), and follow's Typer's guidance on how to report deprecation dates (put it in the docstring).
  • eschews some DRY-driven methods in favor of some explicit RY code (e.g. just register aliases directly).

This PR started getting big, so going to cap it here.

That said, some advances in Click/Typer/Pydantic since this was last touched obviates some of our design.

  • I'm interested in finding a way to use Click's LazyGroups to do lazy loading so that import time doesn't scale with the number of subcommands.

Aside from that, the directory structure should be revisited: the main app is defined in cli/root.py then imported into cli/__init__.py from which every file in cli imports that app and then attaches its local app to it. tl;dr couldn't get more circular if we tried.

Given more time, I'd refactor into a single app.py that takes on responsibility for importing and doing the attaching and registration. Makes it easy to inspect the command space and ensure aliasing is done properly, etc.

Copy link

codspeed-hq bot commented Dec 31, 2024

CodSpeed Performance Report

Merging #16560 will not alter performance

Comparing cleanup-cli (92f7a88) with main (5f5e4a6)

Summary

✅ 3 untouched benchmarks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant