feat(derive): Specify defaults by native expressions

[epage]

Right now

• default_value="something" is a raw method
• default_value uses native types

This commit splits the meanings

• default_value="something" is a raw method
• default_value_t uses T::default()
• default_value_t=expr uses an expression that evaluates to T

This is meant to mirror the value_of / value_of_t API.

At the moment, this is limited to T: Display to work with clap's
default system. Something we can look at in the future is a way to
loosen that restriction. One quick win is to specialize when arg_enum
is set. The main downside is complicating the processing of attributes
because it then means we need some processed before others.

Since this builds on claps existing default system, this also means
users do not get any performance gains out of using default_value_t,
since we still need to parse it but we also need to convert it to a
string.

Fixes #1694

[epage]

@pksunkara FYI looks like this is another sporadic failure: https://github.com/clap-rs/clap/runs/3183588763?check_suite_focus=true

[epage]

@pksunkara CI failure is in src/build/app/mod.rs which this PR doesn't touch

[pksunkara]

Looks good, but we don't really need default_value_t. Everything can be under default_value.

[pksunkara]

Can you use something other than 0 or 1 here?

[epage]

What is it you feel missing with using 0/1 that I should cover?

For the raw case, "0" is non-default. 1 would never show up from defaults.

[pksunkara]

It's just we are testing for 0 & 1 and I wanted this to be "2"

[epage]

Looks good, but we don't really need default_value_t. Everything can be under default_value.

Could you clarify what you intend for how all of this would work under default_value?

We need to distinguish intent between a raw default and a typed default, Personally, I'm unsure of doing that based purely on string literal vs not string literal which is my only guess as to what you intend.

[pksunkara]

Yup, since these are macros, we can use the freedom of syntax.

Let's do

• default_value="something" is a raw method
• default_value uses T::default()
• default_value=expr uses an expression that evaluates to T

[epage]

Yup, since these are macros, we can use the freedom of syntax.

Let's do

* `default_value="something"` is a raw method

* `default_value` uses `T::default()`

* `default_value=expr` uses an expression that evaluates to `T`

default_value="something" is an expression. Overloading intent like this is generally
confusing and brittle.

[pksunkara]

"raw method" is nomenclature in clap derive that denotes it is simply forwarded to clap.

Overloading intent like this is generally confusing and brittle.

Hmm.. I do agree on that. But, that is what structopt has been doing and what default_value_t you did was doing too.

These attributes will be hardest to migrate for end users, which is why I was suggesting to keep the original default_value. And since we are already overloading it, let's just use it for =expr too.

---

Reference on how structopt behaves currently: https://docs.rs/structopt/0.3.22/structopt/#default-values

---

Btw, I removed the underlying issue from 3.0 milestone because I mistakenly tagged it originally.

[epage]

what default_value_t you did was doing too.

My change does not have overloaded intent. default_value will exclusively work with pre-parsed data. default_value_t works exclusively with parsed data.

As I think we should make this breaking change, I think this is still relevant for 3.0. As I said, this makes clap more self-consistent and removes ambiguity which will help on-boarding. We have to weigh both migration and future adoption. As an end-user, this is relatively easy change to migrate to.

[pksunkara]

I am now convinced. Let's remove the special case handling of DefaultValue then in this PR since it just gets forwarded to Method. We don't need this variant anymore.

[epage]

PR updates include

• Making the test parellels more obvious
• Moving away from using a user-defined default that looks like a Default::default
• Removed DefaultValue
• Added an abort (with UI test) to help users migrate

[pksunkara]

cargo-up should be able to handle this easily without us needing the abort. This should be deleted when I finish the structopt to clap upgrader then.

[epage]

I personally would find this useful because I'm unlikely to download a special tool just to handle a library migration. The likelihood of downloading is proportionate to the number of CLIs I maintain.