uv icon indicating copy to clipboard operation
uv copied to clipboard

Display the default value of `--link-mode` in help

Open eth3lbert opened this issue 1 year ago • 8 comments

Summary

Part of #6173 .

Test Plan

Inspect the help output for the following commands.

cargo run pip install --help image

cargo run pip sync --help image

cargo run pip venv --help image

eth3lbert avatar Aug 18 '24 16:08 eth3lbert

Based on the CI failure


running 1 test
━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ Snapshot Summary ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━
Snapshot: tool_install_version-3
Source: crates/uv/tests/tool_install.rs:306
────────────────────────────────────────────────────────────────────────────────
Expression: fs_err::read_to_string(tool_dir.join("black").join("uv-receipt.toml")).unwrap()
────────────────────────────────────────────────────────────────────────────────
-old snapshot
+new results
────────────┬───────────────────────────────────────────────────────────────────
    4     4 │     { name = "blackd", install-path = "[TEMP_DIR]/bin/blackd" },
    5     5 │ ]
    6     6 │ 
    7     7 │ [tool.options]
    8       │-exclude-newer = "2024-03-25T00:00:00Z"
          8 │+exclude-newer = "2024-03-25T00:00:00Z"
          9 │+link-mode = "hardlink"
────────────┴───────────────────────────────────────────────────────────────────

It seems the tool receipt will record the given options. Therefore, we should only set the value when it differs from the default.

eth3lbert avatar Aug 18 '24 16:08 eth3lbert

Also note that the default value varies by platform. We might need some help text about this to prevent confusion when reading the cli reference on the site.

eth3lbert avatar Aug 18 '24 16:08 eth3lbert

I'm not sure how we should handle the snapshot of cli references, as the output depends on the platform it's generated on. :sweat_smile:

eth3lbert avatar Aug 18 '24 17:08 eth3lbert

Lots of complications to consider here! I'm glad you just started with one option. We're going to be focused on 0.3.0 for a bit here — but after that I can try to help with the CLI reference guide and the snapshots. @charliermarsh is also going to need to take a look regarding the .then_some pattern, I'm not sure what implications this has for our tracking of settings in general.

zanieb avatar Aug 19 '24 14:08 zanieb

Or perhaps for this kind of platform-dependent default value, it would be better to display something similar to the CLI reference? e.g.

      --link-mode <LINK_MODE>                  The method to use when installing packages from the global cache. Defaults to `clone` (also known as Copy-on-Write) on macOS, and `hardlink` on Linux and
                                               Windows [env: UV_LINK_MODE=] [possible values: clone, copy, hardlink, symlink]

eth3lbert avatar Aug 19 '24 14:08 eth3lbert

I think it's okay to show the default value per-platform in the CLI help menu, but yeah we'll want show both in the reference documentation — sounds complicated. Might be easier to always show them both for the sake of snapshots — but hopefully it's only a small amount of snapshots that actually include the help menu and we can just use filters...

zanieb avatar Aug 19 '24 15:08 zanieb

Based on the CI feedback, resetting only for ToolOptions seems more correct. Now, it only fails on cli references :)

eth3lbert avatar Aug 20 '24 01:08 eth3lbert

I also noticed that we could implement the Display for LinkMode to allow setting the default value via default_value = LinkMode::default().to_string(). This way, we wouldn't need to change the type. Both approaches would set the value to the default if it's not provided.

Might be easier to always show them both for the sake of snapshots — but hopefully it's only a small amount of snapshots that actually include the help menu and we can just use filters...

Or we could add it (and other options whose doc already shows more descriptive information about the default value) to a group like doc-hide-default, which indicates that we don't want this to be displayed in the cli reference and allows us to omit this default value during generation.

eth3lbert avatar Aug 20 '24 09:08 eth3lbert

As more reference, I had to undo the default value in https://github.com/astral-sh/uv/pull/6835

We don't have access to ArgMatches, though maybe we could refactor things so we do.

zanieb avatar Aug 30 '24 12:08 zanieb

As more reference, I had to undo the default value in #6835

We don't have access to ArgMatches, though maybe we could refactor things so we do.

I guess the following should work:

diff --git a/crates/uv/src/lib.rs b/crates/uv/src/lib.rs
index 080ed988..c5426578 100644
--- a/crates/uv/src/lib.rs
+++ b/crates/uv/src/lib.rs
@@ -1265,0 +1267,15 @@ async fn run_project(
+fn get_given_args(matches: &clap::ArgMatches) -> std::collections::HashSet<&clap::Id> {
+    let mut args = matches;
+    while let Some((_sub_cmd, sub_args)) = args.subcommand() {
+        args = sub_args;
+    }
+    args.ids()
+        .filter(|id| {
+            !matches!(
+                args.value_source(id.as_str()),
+                Some(clap::parser::ValueSource::DefaultValue)
+            )
+        })
+        .collect()
+}
+
@@ -1278,3 +1294,6 @@ where
-    // `std::env::args` is not `Send` so we parse before passing to our runtime
-    // https://github.com/rust-lang/rust/pull/48005
-    let cli = match Cli::try_parse_from(args) {
+    let cmd = Cli::command();
+    let matches = cmd.get_matches_from(args);
+    let cli = match clap::FromArgMatches::from_arg_matches(&matches) {
+        // `std::env::args` is not `Send` so we parse before passing to our runtime
+        // https://github.com/rust-lang/rust/pull/48005
+        // let cli = match Cli::try_parse_from(args) {

This should not alter the way uv currently parses arguments but provide us with the ability to retrieve given arguments from ArgMatches.

eth3lbert avatar Aug 30 '24 17:08 eth3lbert

I think we may just need to keep this as documentation and not as a structured Clap default. As-is (in this PR) it's incorrect in the generated reference too, since it's dynamic.

charliermarsh avatar Oct 09 '24 22:10 charliermarsh

I'm going to close for now but let me know if you want to follow-up in some way. Thank you for your work here.

charliermarsh avatar Oct 09 '24 22:10 charliermarsh