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

test out cli refactor #16535

Draft
wants to merge 18 commits into
base: main
Choose a base branch
from
Draft

test out cli refactor #16535

wants to merge 18 commits into from

Conversation

aaazzam
Copy link
Collaborator

@aaazzam aaazzam commented Dec 29, 2024

This builds on #16496, which introduced a dedicated artifacts module for client methods. This PR does a few things:

  • inserts the console and get_client into the click context's meta argument, so that downstream methods don't have to constantly reason about imports.
  • consolidates and colocates the "business logic" part of the typer command alongside the client methods.

this gives us a path to have a CLI that can invoke one subcommand without having to import everything for every subcommand.

@aaazzam
Copy link
Collaborator Author

aaazzam commented Dec 29, 2024

@desertaxle is this also your love language?

@desertaxle
Copy link
Member

I'm having trouble seeing the benefit of collocating CLI logic with the client. How would prefect.client.orchestration._artifacts.cli integrate with our existing CLI plumbing?

@aaazzam
Copy link
Collaborator Author

aaazzam commented Dec 31, 2024

I feel like half of our prefect {{comand_group}} {{comand}} is a wrapper around client.{{command_group}}.{{comand}}. This PR is kind of a toss of co-locating those things, but starting to see yeah that typer.Option living outside of a CLI namespace is kinda wonky.

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.

2 participants