-
Notifications
You must be signed in to change notification settings - Fork 388
feat: Add new (minimal) pixi_api abstraction crate #4546
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
Conversation
6a92fa8 to
27b942b
Compare
|
After sleeping on it for another night and giving it some more thought, here are some points that could become problematic:
|
As far as I can tell, you are nearly there: pub(crate) async fn init<I: Interface>(interface: &I, options: InitOptions) -> miette::Result<()> {This looks good, but For the CLI, you then have a separate struct that implements |
c849169 to
cab7cb8
Compare
869c153 to
a0fa7e1
Compare
a0fa7e1 to
8e6eb3d
Compare
|
@Hofer-Julian @haecker-felix This looks good! Note that this is also basically what the |
Yes, that's true. I tried to use I think it's better to wait till Overview how we can abstract the current CLI information/data via
|
|
Self-Review: Do we need the But in the case of |
|
Small refactor:
And I ported |
Hofer-Julian
left a comment
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 had a few comments, but it looks good in general!
| workspace | ||
| .workspace | ||
| .value | ||
| .workspace | ||
| .name | ||
| .expect("workspace name must have been set") |
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.
Why not just use name here?
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 assume you mean display_name()?
Hofer-Julian
left a comment
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.
Great work!
High Level Goal:
For this PR I'll limit myself to
pixi initandpixi reinstall.Proposal
Reuse already existing CLIArgstructs, refactor them into*Optionsstructs (e.g.InitOptions)Add some builder pattern for them, so you can use them more nicely (e.g. by usingderive_buildercrate) (?TODO?)Theclapspecific macros could be gated behind aclifeature flag, so non-cli consumers don't getclapas dependency (?TODO?)This way we would get an API which is very close to what's already possible with the CLI interfaceThis should not prevent us from adding further APIs in the future that are not exposed by the Pixi CLI, if that becomes necessary.-> No, turned out that it is better to use separate option structs which are fully independent of the CLI
Argstructs.Next step is taking care of the CLI
execute()functions. A major challenge is that the CLIexecute()functions not only contain CLI-specific code, but also large parts of the business logic itself.First, I tried to strictly separate the business logic from the user interactions (for the example
pixi init). But I have the feeling that this is not the right approach.eprintln!(...)), same with interactions (dialoguer)Alternatively (what this PR does), we can also simply use the existing logic as it is and replace the CLI-specific parts with a generic
Interfacetrait. The specific implementation of theInterfacetrait can then determine whether and how messages, interactions, and similar are handled / presented to the user.Example for
pixi init:pixi/crates/pixi_api/src/init/mod.rs
Line 191 in c849169
Very barebone/simple example for the CLI interface (very likely not sufficient yet, probably needs more context so the
Interfaceimplementors can display this in a useful way to the user):Interfacetrait to benefit directly from potentialpixi_apiimprovements / fixes