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

default_value from lazy Default #150

Closed
swfsql opened this issue Nov 24, 2018 · 11 comments · Fixed by #312
Closed

default_value from lazy Default #150

swfsql opened this issue Nov 24, 2018 · 11 comments · Fixed by #312
Labels
enhancement We would love to have this feature! Feel free to supply a PR

Comments

@swfsql
Copy link

swfsql commented Nov 24, 2018

(Hello, I'm still not sure if this should be on structopt or clap-rs..),

This is a feature request so default_value may be used without any value (like short/long), and be lazily derived from Default itself.

Quick example:

from this:

#[derive(Clone, Debug, StructOpt)]
pub struct Config {
    #[structopt(raw(default_value = "&DEFAULT_CONFIG_BIND_ADDR_STR"))]
    pub bind_addr: SocketAddr,
}

impl Default for Config {
    fn default() -> Self {
        Self {
            bind_addr: FromStr::from_str("127.0.0.1:8080").unwrap()
        }
    }
}

// interface between `Default::default()` and (structopt) `default_value`
lazy_static! {
    static ref DEFAULT_CONFIG: Config = Config::default();
    static ref DEFAULT_CONFIG_BIND_ADDR_STR: String = DEFAULT_CONFIG.bind_addr.to_string();
    // (for fields that has no to_string(), I'm "manually" implementing (interfacing) it with some serde serialization by implementing Display)
}
// lazy_static is needed so &'static str may be used. I don't know how different parsing choices would be dealt with..

into this:

#[derive(Clone, Debug, StructOpt)]
#[structopt(default_value)]
pub struct Config {
    pub bind_addr: SocketAddr,
}

impl Default for Config {
    fn default() -> Self {
        Self {
            bind_addr: FromStr::from_str("127.0.0.1:8080").unwrap()
        }
    }
}

Sorry in advance if this is actually a bad design choice (then I should have no intention of asking for such feature), and that I can't really point into any macro derivation specifics. It's magic !!

@TeXitoi
Copy link
Owner

TeXitoi commented Nov 25, 2018

See #142.

You give me a possible solution: supporting arg less default_value when the type implement Default and ToString.

You need only one lazy_static, doing Type::default().to_string()

@swfsql
Copy link
Author

swfsql commented Nov 25, 2018

I did a slightly larger workaround because, for example, using SocketAddr:: default would prevent different socket addresses of having different default values. Their default value would instead come from the parent/grandparent/etc (in the wrapping sense) types.
If it is that so, getting a string out of Config directly would not necessarily give me the default value of inner types, or I think it would be hard to find out their values..? (edit: I mean that I had to first get the default struct (of a Config), then access it's fields and only then get it as a string. I didn't get as a string directly from the Config)

@TeXitoi
Copy link
Owner

TeXitoi commented Nov 27, 2018

Your proposed design have problematic limitation:

  • the config must implement default, that may not be wanted
  • all the parameter will have a default value

And then, structopt has no way to know if a type implement Default, thus you'll need extra annotation to use the default implementation of the config.

What is your intend? Having the ability to know the default value of a field? Skip the lazy_static usage?

@TeXitoi TeXitoi added the enhancement We would love to have this feature! Feel free to supply a PR label Nov 27, 2018
@CreepySkeleton
Copy link
Collaborator

CreepySkeleton commented Dec 3, 2019

You give me a possible solution: supporting arg less default_value when the type implement Default and ToString.

I'm 100% agree with this design, can I get to implementing it?

This will also fix #142

@TeXitoi
Copy link
Owner

TeXitoi commented Dec 3, 2019

OK.

@swfsql
Copy link
Author

swfsql commented Dec 29, 2019

Thanks for the implementation, @CreepySkeleton !

@TeXitoi I noticed I didn't reply your question, my apologies.

What is your intend? Having the ability to know the default value of a field? Skip the lazy_static usage?

My intent was related to the situation (at that time): I'd have multiple xxx_addr fields, not only bind_addr, and they all would be SocketAddr. So getting the &str value from SocketAddr::default() - in fact, I'm not sure if such implementation exists for SocketAddr - for all of those fields would no work well for me, because they would be different SocketAddr object values.

So my workaround back then was to have a Default for the Config itself, where each field would have their own values set (including fields with had the same type), and then I'd access each field from this default object, then convert it to &str and then finally pass each as a default_value.

I must say that I currently don't continue working on that same project, so it happens that I don't apply this workaround anymore; and I'm also happy with the resolution that you guys designed and implemented!

edit: I actually suspect that it would be possible to have a different newtype wrapper for each of the SocketAddr field, each of which would implement a Default (the actual value that I wanted for each field). So the implementation that you guys worked on, plus this workaround I just described could also be used to solve my particular situation back then!

edit2: another solution - but this would require further design/implementation - would be to not necessarily use de constructor Default::default, but some other function with a compatible signature - fn() -> Self where Self: ToString, and then call such function and proceed to the ToString mapping. This is one idea, in case you guys are interested in tackle with this feature in this direction!

@CreepySkeleton
Copy link
Collaborator

Thanks for the implementation, @CreepySkeleton !

You're welcome!

edit2: another solution - but this would require further design/implementation - would be to not necessarily use de constructor Default::default, but some other function with a compatible signature - fn() -> Self where Self: ToString, and then call such function and proceed to the ToString mapping.

Hmm, Could you please clarify what it would look like? Ideally, show us some pseudocode, an example of such #[derive(StructOpt)].

This is one idea, in case you guys are interested in tackle with this feature in this direction!

For now, I would like to push #314 over the finish line, make a new patch release, and then move to clap_derive and clap. All the future work will/would be done in clap_derive.

@swfsql
Copy link
Author

swfsql commented Dec 30, 2019

@CreepySkeleton sure!

something like:

#[derive(Clone, Debug, StructOpt)]
pub struct Config {
    #[structopt(default_value_fn = "fn1")]
    pub bind_addr: SocketAddr,
    #[structopt(default_value_fn = "fn2")]
    pub bind_addr2: SocketAddr,
    #[structopt(default_value_fn = "Default::default")]
    pub bind_addr3: SocketAddr,
}
fn fn1() -> SocketAddr { FromStr::from_str("127.0.0.1:8080").unwrap() }
fn fn2() -> SocketAddr { FromStr::from_str("127.0.0.1:8081").unwrap() }

This comes from the notion that one could reference function names, like how #[serde(rename(serialize = "ser_name"))] behaves; and then, the Self type resulting from such function only would have to implement ToString.

@CreepySkeleton
Copy link
Collaborator

@swfsql I see, so it's about being able to

  • skip explicit .to_string() call
  • do away with requirement for the Default implementation
  • probably skip one extra FromStr parsing call.

I agree this may be worth implementing, but I'd like to postpone it until after the release of clap_derive.

This comes from the notion that one could reference function names, like how #[serde(rename(serialize = "ser_name"))] behaves; and then, the Self type resulting from such function only would have to implement ToString.

Quite frankly, we could implement virtually (almost) any syntax here, not just #[structopt(method = "lit_str")]. Personally, I would go for #[structopt(default = path::to::func)].

serde uses string literals because, at the time it was being implemented, only string literals were allowed after the = sign. We are free from such a restriction nowadays.

@swfsql
Copy link
Author

swfsql commented Dec 30, 2019 via email

@CreepySkeleton
Copy link
Collaborator

@swfsql FYI, rust-lang/rust#55208

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement We would love to have this feature! Feel free to supply a PR
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants