Switch to generating completions at runtime #393
Conversation
fgimian
force-pushed
the
generate-completions-at-runtime
branch
from
cbfa2c0 to
c12d32e
Compare
|
Thanks! I'm not sure about all the changes here but the completion system can use another look. Shipping this feature by default might be reasonable but I'd like to know how much bloat it adds. What's the size of the release binary before and after your change? (Since there aren't any transitive dependencies besides I'd prefer to keep bundling the completions in the repo/tarballs regardless since it makes things easier for users and distro package maintainers. Making this an option is cleaner than the weird subcommand solution but I think consistency between completions and man pages is even more important. ripgrep has a
Do the Elvish completions work? I excluded them because I tested them and they didn't seem to, see the comment in the old code. That was quite a while ago though. Apparently nushell completions now use a separate crate, are you interested in looking into that? |
|
Thank you so much for the quick reply 😄
Interesting point. I created a branch on my fork which split the generation feature into two so I could test how much larger the executable would be with man page generation, completion generation and both. I'm on Windows but also tested in WSL to get an idea how large the binaries would be on Linux. Here are the results: Windows Linux Personally, I wouldn't be fussed at all about roughly another 300 kb on an 8 or 10 MB binary, but I understand if you feel differently. In my opinion, we should just remove the feature-flag and bundle both with the shipped binary.
Ah yep, that makes sense. I'll refactor my PR to allow for this.
I really like this idea, I think maybe also having a xh --generate=man
xh --generate=complete-bash
xh --generate=complete-zsh
...
xh --generate=man --generate-to=man-pages
xh --generate=complete-bash --generato-to=completions
xh --generate=complete-all --generate-to=completions # complete-all will only be supported when the --generate-to option is usedThis solution gives the best of both worlds as it will mean we can continue to generate completions with a single command as is done today, with automatic file naming. But also, it will give those installing via
Ah, apologies, I did miss that comment in the code. Interestingly though, I just tested the Elvish completions and they appear to work just fine:
I suppose it's possible there was an issue with the generation for Elvish in a prior version of
I have tried Nushell many times and really want to switch to it, but the way the completion system works just feels off to me 😄 However, in saying that, I have absolutely no issue including Nushell completions as part of this PR. Of course, the binary size goes up a pinch with this added: Windows Linux If it were me, I'd also remove the special case for long help: e.g. match raw_method_or_url.as_str() {
"help" => {
// opt-out of clap's auto-generated possible values help for --pretty
// as we already list them in the long_help
app = app.mut_arg("pretty", |a| a.hide_possible_values(true));
app.print_long_help().unwrap();
safe_exit();
}And move this into a dedicated option as well. This way I'll go ahead with my ideas in this branch in the meantime, but happy to tweak as needed based on your feedback. Cheers |
fgimian
force-pushed
the
generate-completions-at-runtime
branch
9 times, most recently
from
50ed757 to
d021abf
Compare
|
Hey there @blyxxyz, I think this is now ready for review. This PR now does the following:
As per the PR, here are the new equivalent commands for generating everything: - cargo run --all-features -- generate-completions completions && cargo run --all-features -- generate-manpages doc
+ cargo run --all-features -- --generate complete-all --generate-to completions && cargo run --all-features -- --generate man-pages --generate-to docAnd now for some questions / concerns I wanted to ask about:
e.g. this is how filenames would look before And after my changes:
Now, since I'm generating man pages without running that method in More specifically, here is a full diff: --- doc1/xh.1 2024-12-31 14:16:05.183895500 +1100
+++ doc2/xh.1 2024-12-31 14:39:54.343134400 +1100
@@ -1,4 +1,4 @@
-.TH XH 1 2024-10-12 0.23.0 "User Commands"
+.TH XH 1 2024-12-31 0.23.0 "User Commands"
.SH NAME
xh \- Friendly and fast tool for sending HTTP requests
@@ -92,16 +92,22 @@
(default) Serialize data items from the command line as a JSON object.
Overrides both \-\-form and \-\-multipart.
+
+[possible values: true, false]
.TP 4
\fB\-f\fR, \fB\-\-form\fR
Serialize data items from the command line as form fields.
Overrides both \-\-json and \-\-multipart.
+
+[possible values: true, false]
.TP 4
\fB\-\-multipart\fR
Like \-\-form, but force a multipart/form\-data request even without files.
Overrides both \-\-json and \-\-form.
+
+[possible values: true, false]
.TP 4
\fB\-\-raw\fR=\fIRAW\fR
Pass raw request data without extra processing.
@@ -153,12 +159,18 @@
.TP 4
\fB\-h\fR, \fB\-\-headers\fR
Print only the response headers. Shortcut for \-\-print=h.
+
+[possible values: true, false]
.TP 4
\fB\-b\fR, \fB\-\-body\fR
Print only the response body. Shortcut for \-\-print=b.
+
+[possible values: true, false]
.TP 4
\fB\-m\fR, \fB\-\-meta\fR
Print only the response metadata. Shortcut for \-\-print=m.
+
+[possible values: true, false]
.TP 4
\fB\-v\fR, \fB\-\-verbose\fR
Print the whole request as well as the response.
@@ -173,9 +185,13 @@
Print full error stack traces and debug log messages.
Logging can be configured in more detail using the `$RUST_LOG` environment variable. Set `RUST_LOG=trace` to show even more messages. See https://docs.rs/env_logger/0.11.3/env_logger/#enabling\-logging.
+
+[possible values: true, false]
.TP 4
\fB\-\-all\fR
Show any intermediary requests/responses while following redirects with \-\-follow.
+
+[possible values: true, false]
.TP 4
\fB\-P\fR, \fB\-\-history\-print\fR=\fIFORMAT\fR
The same as \-\-print but applies only to intermediary requests/responses.
@@ -187,6 +203,8 @@
.TP 4
\fB\-S\fR, \fB\-\-stream\fR
Always stream the response body.
+
+[possible values: true, false]
.TP 4
\fB\-o\fR, \fB\-\-output\fR=\fIFILE\fR
Save output to FILE instead of stdout.
@@ -195,9 +213,13 @@
Download the body to a file instead of printing it.
The Accept\-Encoding header is set to identify and any redirects will be followed.
+
+[possible values: true, false]
.TP 4
\fB\-c\fR, \fB\-\-continue\fR
Resume an interrupted download. Requires \-\-download and \-\-output.
+
+[possible values: true, false]
.TP 4
\fB\-\-session\fR=\fIFILE\fR
Create, or reuse and update a session.
@@ -221,9 +243,13 @@
.TP 4
\fB\-\-ignore\-netrc\fR
Do not use credentials from .netrc.
+
+[possible values: true, false]
.TP 4
\fB\-\-offline\fR
Construct HTTP requests without sending them anywhere.
+
+[possible values: true, false]
.TP 4
\fB\-\-check\-status\fR
(default) Exit with an error status code if the server replies with an error.
@@ -231,9 +257,13 @@
The exit code will be 4 on 4xx (Client Error), 5 on 5xx (Server Error), or 3 on 3xx (Redirect) if \-\-follow isn't set.
If stdout is redirected then a warning is written to stderr.
+
+[possible values: true, false]
.TP 4
\fB\-F\fR, \fB\-\-follow\fR
Do follow redirects.
+
+[possible values: true, false]
.TP 4
\fB\-\-max\-redirects\fR=\fINUM\fR
Number of redirects to follow. Only respected if \-\-follow is used.
@@ -276,11 +306,10 @@
[possible values: auto, tls1, tls1.1, tls1.2, tls1.3]
.TP 4
-\fB\-\-native\-tls\fR
-Use the system TLS library instead of rustls (if enabled at compile time).
-.TP 4
\fB\-\-https\fR
Make HTTPS requests if not specified in the URL.
+
+[possible values: true, false]
.TP 4
\fB\-\-http\-version\fR=\fIVERSION\fR
HTTP version to use.
@@ -301,9 +330,13 @@
.TP 4
\fB\-4\fR, \fB\-\-ipv4\fR
Resolve hostname to ipv4 addresses only.
+
+[possible values: true, false]
.TP 4
\fB\-6\fR, \fB\-\-ipv6\fR
Resolve hostname to ipv6 addresses only.
+
+[possible values: true, false]
.TP 4
\fB\-I\fR, \fB\-\-ignore\-stdin\fR
Do not attempt to read stdin.
@@ -311,20 +344,25 @@
This disables the default behaviour of reading the request body from stdin when a redirected input is detected.
It is recommended to pass this flag when using xh for scripting purposes. For more information, refer to https://httpie.io/docs/cli/best\-practices.
+
+[possible values: true, false]
.TP 4
\fB\-\-curl\fR
Print a translation to a curl command.
For translating the other way, try https://curl2httpie.online/.
+
+[possible values: true, false]
.TP 4
\fB\-\-curl\-long\fR
Use the long versions of curl's flags.
+
+[possible values: true, false]
.TP 4
\fB\-\-help\fR
Print help.
-.TP 4
-\fB\-V\fR, \fB\-\-version\fR
-Print version.
+
+[possible values: true, false]
.SH EXIT STATUS
.TP 4Cheers |
fgimian
force-pushed
the
generate-completions-at-runtime
branch
4 times, most recently
from
db53e09 to
6e02d33
Compare
There was a problem hiding this comment.
Thanks for adding nushell and testing elvish! I'm guessing it got fixed in clap-rs/clap@6d27488.
300K might be OK. But it does add up, it's not the first time we've said OK to another 300K.
Can you commit the new versions of the generated files so they're visible in the PR diff?
If it were me, I'd also remove the special case for long help
I'd rather not do this. I'm not sure that it's better, but even if it were, we probably have users who are used to xh help and it's just not worth the disruption. (Mind that xh --help already mentions xh help, it's just added manually at the end rather than listed among the options. Which puts it very prominently into view in the terminal.)
[possible values: true, false]
Huh, weird! Not useful to users since they aren't actually able to pass those values so we shouldn't show those.
I went down a rabbit hole to figure out what was going on. The solution turns out to be to call app.build(). That will resolve all the internal information that's required to make get_possible_values() behave properly. (This happens implicitly when you parse a command line or render the help text.)
The best place to call app.build() is probably at the end of fn into_app().
This actually works a little better on Windows as it omits the
*.exeextension from completion files.
Nice!
src/cli.rs
Outdated
| CompleteNushell, | ||
| CompletePowershell, | ||
| CompleteZsh, | ||
| CompleteAll, |
There was a problem hiding this comment.
It makes more sense to me to leave out this variant and have --generate-to generate man pages as well as all completions and conflict with --generate.
There was a problem hiding this comment.
I'm more than happy to do this, but I think that it will need two hidden options if you wish to be able to specify the output path like the current version. we could do --generate-completions-to and --generate-man-to and have them hidden. Just be aware that hiding options in clap doesn't seem to remove them from completions. 😄
Alternatively, we could go with something like --generate-all and have the output paths (doc and completions) hard-coded. We could also have that particular option behind a feature-flag similar to how it was previously to ensure normal users can not run it at all. Personally I think that it should be behind a feature flag if the output paths are hard-coded.
src/main.rs
Outdated
| @@ -62,6 +67,50 @@ fn get_user_agent() -> &'static str { | |||
|
|
|||
| fn main() { | |||
| let args = Cli::parse(); | |||
| let bin_name = args.bin_name.clone(); | |||
|
|
|||
| if let Some(generate) = args.generate { | |||
There was a problem hiding this comment.
Can you split this section off into its own function that's called from run (like to_curl)? That way we'll have logging set up and don't need to duplicate the error reporting.
There was a problem hiding this comment.
Oh yes, I should have done that, fixed. I matched the method used for to_curl but am wondering if I should go with a slightly different approach when passing the args.
At present, I'm doing this to match to_curl:
...
if args.generate.is_some() {
generation::generate(args)?;
return Ok(0);
}
...
pub fn generate(args: Cli) -> Result<()> {
if let Some(generate) = args.generate {
let bin_name = args.bin_name.clone();
let mut app = Cli::into_app();
let out_directory = args.generate_to.as_deref();
match generate {
Generate::CompleteBash => {
generate_completions(&bin_name, &mut app, Shell::Bash, out_directory)
}
...
}
} else {
Ok(())
}
}Of course, I could do something more like this:
...
if let Some(generate) = args.generate {
generation::generate(generate, args)?;
return Ok(0);
}
...
pub fn generate(generate: Generate, args: Cli) -> Result<()> {
let bin_name = args.bin_name.clone();
let mut app = Cli::into_app();
let out_directory = args.generate_to.as_deref();
match generate {
Generate::CompleteBash => {
generate_completions(&bin_name, &mut app, Shell::Bash, out_directory)
}
...
}
}Furthermore, I could also pass in bin_name to avoid an extra clone and also just pass in args.generate_to to avoid passing in args at all.
What do you think?
Thank you so much, I'll make the necessary changes tomorrow and let you know when they're ready for review. Hope you have a very happy new year! 🎉 Cheers |
fgimian
force-pushed
the
generate-completions-at-runtime
branch
from
8d6c792 to
0d03009
Compare
|
Just thought I'd respond properly to various points in your comment 😄
No worries at all, done in the latest commit.
Completely understand, reverted this in the latest commit.
Legend! Thank you so much for digging into this and helping me out with it, I did what you mentioned on the latest commit and it resolved the problem. |
fgimian
force-pushed
the
generate-completions-at-runtime
branch
2 times, most recently
from
7ec6168 to
e6b6322
Compare
|
@blyxxyz Thank you so much again for the incredibly detailed feedback and review. There are just two points remaining which I'd love further feedback on, please see responses to your comments in the review. I added a second commit with changes since the review to make it easier for you to see what I have changed, but happy to squash all these commits down to one once we're ready to merge. Cheers |
|
One more additional thought I had was leaving |
|
Ah, I forgot that the completions and the man page go in different directories... Since it's only really useful for packaging anyway I think it makes sense to stop using |
|
Sounds good to me. We can skip the convenience script if we document the commands for generating completions and man-pages in |
fgimian
force-pushed
the
generate-completions-at-runtime
branch
2 times, most recently
from
553bc90 to
1a2314f
Compare
…or Elvish and Nshull
fgimian
force-pushed
the
generate-completions-at-runtime
branch
from
1a2314f to
d01774b
Compare
|
@blyxxyz @ducaale Thank you so much for the feedback and your patience with this. I've updated my work based on the feedback provided (i.e. Please give this a fresh review when you have some time and let me know what you think. Cheers |
|
Thank you both for your detailed review. I've pushed one more commit incorporating all the additional changes requested. I am happy to squash the commits before merge if it all looks good for you, or I'm guessing GitHub will allow you to squash via the site when merging. Cheers |
Hey there, absolutely love xh! 😄
This PR simplifies generation of shell completions. Rather than being locked behind a feature-flag and requiring generation at compile-time and bundling, shell completions may now be generated via
xh --completions <shell>at runtime similar to various other tools.This also automatically will provide completions for all shells provided by clap (including Elvish which was previously unavailable). Furthermore, the
--completionsoption will clearly appear in the help and avoid the potentialerror: generate-completions requires enabling man-completion-gen featurein release versions of xh when run withxh generate-completions.Kindly let me know if you are happy with this new behaviour and if you have any questions
Cheers
Fotis