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

derive: Store indices, value source next to field values #3846

Open
2 tasks done
stepancheg opened this issue Jun 16, 2022 · 8 comments
Open
2 tasks done

derive: Store indices, value source next to field values #3846

stepancheg opened this issue Jun 16, 2022 · 8 comments
Labels
A-derive Area: #[derive]` macro API C-enhancement Category: Raise on the bar on expectations S-waiting-on-design Status: Waiting on user-facing design to be resolved before implementing

Comments

@stepancheg
Copy link
Contributor

Please complete the following tasks

Clap Version

3.1.8

Describe your use case

Be able to write something like this:

#[clap(
  indices,
)]
pub rrrr: Vec<(usize, String)>,

Currently to obtain indices, we need to work with ArgMatches::indices_of which is hard to work with when a command has subcommands which have other subcommands: it's too easy to miss some subcommand call, and indices won't return what's expected (especially when subcommands share flags, but at different nesting level).

Describe the solution you'd like

.

Alternatives, if applicable

No response

Additional Context

No response

@stepancheg stepancheg added the C-enhancement Category: Raise on the bar on expectations label Jun 16, 2022
@epage epage added A-derive Area: #[derive]` macro API S-waiting-on-design Status: Waiting on user-facing design to be resolved before implementing labels Jun 17, 2022
@epage
Copy link
Member

epage commented Jun 17, 2022

Was just mentioning this to someone else that we need an issue for this.

The other piece of metadata we do not expose in the derive API is the ValueSource.

I have some reticence for putting it in the same field due to the complications of parsing and processing more complex types like that.

Another option is to take inspiration from #[clap(from_global)] and generalize it to a #[clap(from(...))] attribute with

  • global, like #[clap((from(global))]
  • index, like #[clap(from(index), id = "foo")]
    • require an explicit id
  • source, like #[clap(from(source), id = "foo")]
    • require an explicit id

@epage epage changed the title derive: Store indices next to field values derive: Store indices, value source next to field values Jun 17, 2022
@stepancheg
Copy link
Contributor Author

require an explicit id

That would be great if correctness of id is checked at compile time.

@epage
Copy link
Member

epage commented Jun 17, 2022

require an explicit id

That would be great if correctness of id is checked at compile time.

Something that could make it easier is #3171. My main hesitancy about it is I feel like derive-macros generating something besides a trait impl is a bit magical from the users perspective (hard to know what exists and how to interact with it).

@chipsenkbeil
Copy link

Going to copy over my discussion into this issue. 😄

@epage for deriving value source, I'd personally be happy with having some wrapper type that gets populated in the derive macro. Having some way to have this baked into the struct would enable me to handle #748 manually as I could still provide a default value for all of my cli arguments, but also manually replace them with a config file I've loaded separately just by knowing if the value is a default value. E.g.

use clap::{Args, Value, parser::ValueSource};

#[derive(Args)]
struct MyArgs {
    /// This has no value source encoded
    #[clap(short, long)]
    pub name: String,

    /// This does have value source encoded
    #[clap(short, long, value_source)]
    pub age: Value<u8>,
}

// when accessing ...
let args: MyArgs = /* ... */;
assert_eq!(args.name, "some name");
assert_eq!(args.age, 37);
assert_eq!(args.age.source, ValueSource::CommandLine);

// if I had a config value I wanted to use only if the arg value came from the default
let age = args.age.replace_if_default(config.age);
assert_eq!(args.age, 28);
use std::ops::{Deref, DerefMut};
use clap::parser::ValueSource;

// Value itself is a wrapper that looks like
#[derive(Copy, Clone)]
struct Value<T> {
    inner: T,
    pub source: ValueSource,
}

impl<T> Value<T> {
    /// Returns the updated value if replaced, otherwise returns the original value
    pub fn replace_if_default(self, inner: T) -> Self {
        if self.source == ValueSource:: DefaultValue {
            Value { inner, source: self.source }
        } else {
            self
        }
    }
}

impl<T> Deref for Value<T> {
    type Target = T;

    fn deref(&self) -> &Self::Target {
        &self.inner
    }
}

impl<T> DerefMut for Value<T> {
    fn deref_mut(&mut self) -> &mut Self::Target {
        &mut self.inner
    }
}

impl<T> From<Value<T>> for T {
    // To enable passing this value around as the actual type
    fn from(value: Value<T>) -> Self {
        value.inner
    }
}

impl<T> PartialEq<T> for Value<T> where T: PartialEq {
    // To enable equality checks
    fn eq(&self, other: &T) -> bool {
        self.inner == other
    }
}

// do this for the other traits ...

Originally posted by @chipsenkbeil in #3578 (reply in thread)

@epage
Copy link
Member

epage commented Jul 14, 2022

@chipsenkbeil Thanks for copying that over!

Having a value_source attribute helps avoid the challenges with inferring what is happening based on types along (unless we use deref specialization).

It does have a scaling issue. Some users might want the source while others might want the index or both. Also the source common to all values while the index is per value.

In exploring the various options, what are your thoughts on the parallel-field idea I had proposed earlier in the thread?

@chipsenkbeil
Copy link

In exploring the various options, what are your thoughts on the parallel-field idea I had proposed earlier in the thread?

@epage are you referring to the idea of #[clap(from(index))] and #[clap(from(value))]? It seems fine to me, but I don't have an understanding of index or the from_global that you were referencing earlier.

@epage
Copy link
Member

epage commented Jul 25, 2022

Yes.

from_global: clap has the concept of global arguments which allows an argument to be defined on the parent command and it will automatically be defined on all subcommands and the value will be propagated back up to the parent command. Think of flags like those to control logging levels. from_global allows accessing the argument within the subcommand even though it isn't defined in that subcommand.

index: clap tracks the relative order that arguments appeared on the command line. See ArgMatches::indices_of for more details.

claytonrcarter added a commit to claytonrcarter/git-branchless that referenced this issue Nov 5, 2022
Unfortunately, this prevents clap from rendering the default revset in
the help message, but I believe this is the only way to detect if the
user has not not supplied an arg via the derive API.

See clap-rs/clap#3846 fmi on getting the
value_source of an arg via the derive API.
claytonrcarter added a commit to claytonrcarter/git-branchless that referenced this issue Nov 6, 2022
This doesn't do much for us now, but will soon be used to help us determine
what type of smartlog we should be rendering.

Unfortunately, this prevents clap from rendering the default revset in
the help message, but I believe this is the only way (via the derive API)
to detect if the user has or has not not supplied an arg for a particular
value.

See clap-rs/clap#3846 fmi on getting the
value_source of an arg via the derive API.
claytonrcarter added a commit to claytonrcarter/git-branchless that referenced this issue Nov 7, 2022
This doesn't do much for us now, but will soon be used to help us determine
what type of smartlog we should be rendering.

Unfortunately, this prevents clap from rendering the default revset in
the help message, but I believe this is the only way (via the derive API)
to detect if the user has or has not not supplied an arg for a particular
value.

See clap-rs/clap#3846 fmi on getting the
value_source of an arg via the derive API.
claytonrcarter added a commit to claytonrcarter/git-branchless that referenced this issue Nov 7, 2022
This doesn't do much for us now, but will soon be used to help us determine
what type of smartlog we should be rendering.

Unfortunately, this prevents clap from rendering the default revset in
the help message, but I believe this is the only way (via the derive API)
to detect if the user has or has not not supplied an arg for a particular
value.

See clap-rs/clap#3846 fmi on getting the
value_source of an arg via the derive API.
claytonrcarter added a commit to claytonrcarter/git-branchless that referenced this issue Nov 9, 2022
This doesn't do much for us now, but will soon be used to help us determine
what type of smartlog we should be rendering.

Unfortunately, this prevents clap from rendering the default revset in
the help message, but I believe this is the only way (via the derive API)
to detect if the user has or has not not supplied an arg for a particular
value.

See clap-rs/clap#3846 fmi on getting the
value_source of an arg via the derive API.
claytonrcarter added a commit to claytonrcarter/git-branchless that referenced this issue Nov 9, 2022
This doesn't do much for us now, but will soon be used to help us determine
what type of smartlog we should be rendering.

Unfortunately, this prevents clap from rendering the default revset in
the help message, but I believe this is the only way (via the derive API)
to detect if the user has or has not not supplied an arg for a particular
value.

See clap-rs/clap#3846 fmi on getting the
value_source of an arg via the derive API.
arxanas pushed a commit to claytonrcarter/git-branchless that referenced this issue Nov 9, 2022
This doesn't do much for us now, but will soon be used to help us determine
what type of smartlog we should be rendering.

Unfortunately, this prevents clap from rendering the default revset in
the help message, but I believe this is the only way (via the derive API)
to detect if the user has or has not not supplied an arg for a particular
value.

See clap-rs/clap#3846 fmi on getting the
value_source of an arg via the derive API.
arxanas pushed a commit to arxanas/git-branchless that referenced this issue Nov 9, 2022
This doesn't do much for us now, but will soon be used to help us determine
what type of smartlog we should be rendering.

Unfortunately, this prevents clap from rendering the default revset in
the help message, but I believe this is the only way (via the derive API)
to detect if the user has or has not not supplied an arg for a particular
value.

See clap-rs/clap#3846 fmi on getting the
value_source of an arg via the derive API.
@epage
Copy link
Member

epage commented Nov 30, 2022

This issue is unrelated. In the future, feel free to start a Discussion.

In this case, value source is stable / should work. We'll need more details, like a reproduction case. Please create a Discussion with that to not detract from this Issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-derive Area: #[derive]` macro API C-enhancement Category: Raise on the bar on expectations S-waiting-on-design Status: Waiting on user-facing design to be resolved before implementing
Projects
None yet
Development

No branches or pull requests

3 participants