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

RFC: use clap::Command interface instead of procedure macro #585

Closed
Roger-luo opened this issue Feb 19, 2023 · 6 comments
Closed

RFC: use clap::Command interface instead of procedure macro #585

Roger-luo opened this issue Feb 19, 2023 · 6 comments
Labels
enhancement New feature or request
Milestone

Comments

@Roger-luo
Copy link
Contributor

Roger-luo commented Feb 19, 2023

currently juliaup is implemented using the procedure macro, which is much simpler than clap::Command interface, however, this is much less flexible and less modular on creating more complicated CLI applications that contain multiple sub-commands.

For example, cargo uses a much modular structure based on the Command object interface https://github.com/rust-lang/cargo/blob/master/src/bin/cargo/commands/add.rs#L17

On the other hand, although errors in juliaup are handled, but not as exported as different terminal exit codes. This is something can be automatically enabled by a cargo-like CLI project structure.

More specifically, AFAIK there is no way to generate shell completions from procedure macro directly at the moment. The clap_completions crate is based on the ValueHint enum, but there is no way to define ValueHint in the derive macro. https://docs.rs/clap/latest/clap/_derive/index.html

And I can refactor juliaup into clap::Command to eat my own dog food, but just want to know if people want this.

@davidanthoff
Copy link
Collaborator

Hm, so far the simple story that we are using right now has worked well, I haven't really had a situation where I ran into a limitation around it? I am not generally against this, but at the moment I have limited bandwidth to review/do things here, and I think sorting out the remaining issues that are preventing us from making this the default Julia installer is probably a higher priority...

@davidanthoff davidanthoff added this to the Backlog milestone Feb 25, 2023
@Roger-luo
Copy link
Contributor Author

I haven't really had a situation where I ran into a limitation around it? I am not generally against this, but at the moment I have limited bandwidth to review/do things here, and I think sorting out the remaining issues that are preventing us from making this the default Julia installer is probably a higher priority...

That's fully understandable, I think the main missing piece here is I want to have some shell completions but this is definitely not as urgent as other things. Just let me know if in the future you have time to review such a refactor - I don't think it's hard to do tho, just copy-pasting some code from cargo and re-organize the files.

@uncomfyhalomacro
Copy link
Contributor

I will be willing to contribute to this once the remaining issues are fixed.

@davidanthoff davidanthoff added the enhancement New feature or request label May 5, 2023
@sanmai-NL
Copy link

Using procedural macros complicates usage of RUSTFLAGS="-Ctarget-feature=+crt-static".
rust-lang/rust#71651

@fingolfin
Copy link
Member

PR #753 adds support for shell completions without switching away from the clap "derive" interface to the more complicated "builder" interface (i.e. using clap::Command).

So assuming that PR is merged, I think this issue can be closed?

@Roger-luo
Copy link
Contributor Author

it doesn't solve all the issues raised in this thread? but it's prob quite minor.

On the other hand, although errors in juliaup are handled, but not as exported as different terminal exit codes.

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

No branches or pull requests

5 participants