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

Serialize & Deserialize #88

Open
nathanhammond opened this issue Jan 23, 2024 · 10 comments · May be fixed by #114
Open

Serialize & Deserialize #88

nathanhammond opened this issue Jan 23, 2024 · 10 comments · May be fixed by #114

Comments

@nathanhammond
Copy link

nathanhammond commented Jan 23, 2024

There was a straightforward PR thrown up for discussion over at #65, effectively looking for a Serialize and Deserialize implementation for this library. The requests from @epage:

I think more of a case is needed for why a CLI flag needs serialization, particularly the fact that this was done in a raw way (separate verbose, quiet as u8s) - #65 (comment)

More design discussion is needed on this because exposing separate verbose and quiet fields that are u8 does not make sense. - #65 (comment)


Use Cases

Instrumentation

If an entire args object is being passed into tracing it is quite nice to have a Serialize so that derive works. I don't want to have to impl Serialize for Args for this reason. If I don't do some serialization then I end up with Debugand that is super-verbose.

use clap::Parser;
use serde::Serialize;
use svix_ksuid::{Ksuid, KsuidLike};
use tracing::instrument;

mod telemetry;

#[derive(Serialize, Parser, Debug)]
#[command(author, version, about, long_about = None)]
struct Args {
    // #[command(flatten)]
    // verbose: clap_verbosity_flag::Verbosity,
    #[arg(short, long, default_value_t = 1)]
    count: u8,
}

impl Args {
    fn to_json(&self) -> String {
        return serde_json::to_string(&self).expect("");
    }
}

// Main is a very small setup shim, and then gets out of the way.
fn main() -> proc_exit::ExitResult {
    let id = Ksuid::new(None, None);

    telemetry::init(id);
    let args = Args::parse();

    let result = run(id, args);
    proc_exit::exit(result);
}

#[instrument(name = "run()", fields(args = args.to_json(), id = id.to_string()) ret, err)]
fn run(id: Ksuid, args: Args) -> proc_exit::ExitResult {
    proc_exit::Code::SUCCESS.ok()
}

Args From <Elsewhere>

This some-cli gui command spins up a server and opens a browser to an HTML page. Eventually that HTML page makes a network request back to the server, which contains a serialized form of the arguments that would have been passed in. That's going to be JSON (cause it's coming from the web), deserializing into Args (from the above example).

Starts the server:

$ some-cli gui

(Complicated configuration happens in a browser.)

Data from a "configuration finished!" network request passed back in to the CLI entry point:

let args: Args = serde_json::from_str(args)?;
let result = run(id, args);

Design Space

There are basically only two reasonable options here to represent configuration since it is a count.

As Separate u8s

exposing separate verbose and quiet fields that are u8 does not make sense

Given that we have -q and -v[vvv] exposed as separate args this separation doesn't seem particularly egregious. Further, since the struct can be constructed with clap_verbosity_flag::Verbosity::new(2, 4), any serialization that doesn't give you both of those values is lossy.

Being able to represent the error conditions (error: the argument '--quiet...' cannot be used with '--verbose...') that can be triggered by command line usage is reasonable—even though typically it would never get through ::parse().

As A Single i16

Since we have the conflicts_with lockout from the primary clap use case, we could choose to serialize them into a single value, in spite of it being lossy. Take a page from the log mapping, and do quiet as -(count).

An i16 gives us way more bits than we need, but there isn't an i9.

...
-4 => -qqqq
-3 => -qqq
-2 => -qq
-1 => -q
0 => None
1 => -v
2 => -vv
3 => -vvv
4 => -vvvv
...

This approach can't support { quiet: 4, verbosity: 4 } like a manually constructed struct can.


I'm in favor of the separate u8s for representation fidelity reasons and nominate reopening and accepting #65.

@nathanhammond
Copy link
Author

Here is what a remote derive implementation looks like, a lot of hassle and hackery:

[derive(Serialize, Deserialize)]
#[serde(remote = "clap_verbosity_flag::Verbosity")]
struct VerbosityDef {
    #[serde(getter = "clap_verbosity_flag::Verbosity::verbose")]
    pub verbose: u8,
    #[serde(getter = "clap_verbosity_flag::Verbosity::quiet")]
    pub quiet: u8,
}

pub trait VerbosityRemoteGetters {
    fn verbose(&self) -> u8;
    fn quiet(&self) -> u8;
}

impl VerbosityRemoteGetters for clap_verbosity_flag::Verbosity {
    fn verbose(&self) -> u8 {
        let debug_string = format!("{:#?}", self);
        let re = Regex::new(r"verbose: (?<verbose>[\d]+)").unwrap();
        let output = re.captures(&debug_string).unwrap();
        let string_value = output.name("verbose").unwrap().as_str();
        let calc = string_value.parse::<u8>().unwrap();
        calc
    }
    fn quiet(&self) -> u8 {
        let debug_string = format!("{:#?}", self);
        let re = Regex::new(r"quiet: (?<quiet>[\d]+)").unwrap();
        let output = re.captures(&debug_string).unwrap();
        let string_value = output.name("quiet").unwrap().as_str();
        let calc = string_value.parse::<u8>().unwrap();
        calc
    }
}

impl From<VerbosityDef> for clap_verbosity_flag::Verbosity {
    fn from(def: VerbosityDef) -> clap_verbosity_flag::Verbosity {
        clap_verbosity_flag::Verbosity::new(def.verbose, def.quiet)
    }
}

#[derive(Serialize, Deserialize, Parser, Debug)]
#[command(author, version, about, long_about = None)]
struct Args {
    #[serde(with = "VerbosityDef")]
    #[command(flatten)]
    verbose: clap_verbosity_flag::Verbosity,
}

Or, I could just copy/paste out the core of the implementation in fewer lines:

#[derive(clap::Args, Debug, Clone, Default)]
#[command(about = None, long_about = None)]
pub struct Verbosity<L: LogLevel = ErrorLevel> {
#[arg(
long,
short = 'v',
action = clap::ArgAction::Count,
global = true,
help = L::verbose_help(),
long_help = L::verbose_long_help(),
)]
verbose: u8,
#[arg(
long,
short = 'q',
action = clap::ArgAction::Count,
global = true,
help = L::quiet_help(),
long_help = L::quiet_long_help(),
conflicts_with = "verbose",
)]
quiet: u8,
#[arg(skip)]
phantom: std::marker::PhantomData<L>,
}

@epage
Copy link
Member

epage commented Jan 23, 2024

To be clear, are those your use cases or theoretical use cases?

As for the serialization format, I consider any non-textual serialization format to be a non-starter. As a user, entering random numbers in two fields and dealing with how those fields interact makes no sense and I would expect to just say "trace" or similar. The CLI is designed for user convenience / following convention and we hide the individual fields because that is the level of abstraction people should be working with.

@nathanhammond
Copy link
Author

I have both of those use cases in the project I'm working on. You're looking at a trimmed down main.rs from a project that I'm just starting in on.

https://www.wispbuild.com

Designing a query for particular nodes in a huge DAG is hard; it's way easier to present them to the user for clicking/visual feedback of what is selected. They can run multiple times from the GUI context until it is right, and then serialize out the command string for later use.

Sample output for each option in the GUI makes it easy to figure out which options are in use, and applies to both verbose and quiet.

@nathanhammond
Copy link
Author

nathanhammond commented Jan 24, 2024

I don't have super-strong opinions here (I've since dropped this as a dep), but I feel like there is a lot of value in standardizing the underlying tooling layer for the CLI hello world because it creates consistent UX outcomes for users.

The lack of Serialize and Deserialize caused me to eject from that vision.


Not exposing the numerics means that displaying the outcome as a range slider in the HTML GUI still requires mapping it myself from string, making the i16 approach more attractive. But that's not super-materially different since I need the labels anyway. (The web UI would never be two fields, the actual serialization format remains an implementation detail not presented to the end-user.)

@joshka
Copy link
Contributor

joshka commented Sep 9, 2024

I just ran into an actual non theoretical use for where Serialize and Deserialize are useful.

I'm implementing a cli app that can be configured by either clap or loaded from a config file using Figment. This approach is documented at https://docs.rs/figment/latest/figment/#for-cli-application-authors

To load the config / cli args, I have a config struct:

#[derive(Debug, Deserialize, Serialize, Parser)]
pub struct Config {
    /// The port to listen on
    #[arg(short, long, default_value = "3000")]
    pub port: u16,

    /// The default log level
    #[command(flatten)
    pub log_level: Verbosity<InfoLevel>,
}

and a load function:

pub fn load() -> Result<Config> {
    Figment::from(Serialized::defaults(Config::parse()))
        .merge(Toml::file("Config.toml"))
        .extract()
        .wrap_err("Failed to load configuration")
}

This parses the args into a config, serializes the config, and merges these values with any values set in the config file (likely these should be around the other way - config then cli, but the serialization part is the blocker here regardless.)

Would implementing this as a serialization onto the level name be sufficient to move this forward?

@epage
Copy link
Member

epage commented Sep 19, 2024

@joshka to clarify, what you are looking for is the names of the levels to be serialized?

@joshka
Copy link
Contributor

joshka commented Sep 20, 2024

@joshka to clarify, what you are looking for is the names of the levels to be serialized?

Yes, I phrased it poorly, but what I meant to ask was given that you "consider any non-textual serialization format to be a non-starter.", would a PR that (de)serializes Verbosity<InfoLevel> into a string representation of the log level name ("trace", "debug", etc.) be worth investing time into?

@epage
Copy link
Member

epage commented Sep 20, 2024

@joshka the use cases for that kind of solution are much clearer, as demonstrated by the one you listed. If its behind a serde feature, I'm good with it.

joshka added a commit to joshka/clap-verbosity-flag that referenced this issue Sep 20, 2024
This commit adds serialization and deserialization support for the
Verbosity type. The verbosity is serialized as an optional log level
that represents the equivalent log level given the number of verbose and
quiet flags. When deserializing, the log level is converted into a
number of verbose and quiet flags, which keeps one of the flags at 0 and
the other at the difference between the log level and the default log
level.

The internal representation in calculations has been changed from i8 to
i16 to prevent overflows when performing calculations using u8 values.

Fixes: clap-rs#88
joshka added a commit to joshka/clap-verbosity-flag that referenced this issue Sep 21, 2024
This commit adds serialization and deserialization support for the
Verbosity type. The verbosity is serialized using the log::LevelFilter
enum that represents the equivalent number of verbose and quiet flags.

The serialized value is the uppercase variant of the enum variant.
Deserialing is case-insensitive.

Fixes: clap-rs#88
joshka added a commit to joshka/clap-verbosity-flag that referenced this issue Sep 21, 2024
This commit adds serialization and deserialization support for the
Verbosity type. The verbosity is serialized using the log::LevelFilter
enum that represents the equivalent number of verbose and quiet flags.

The serialized value is the uppercase variant of the enum variant.
Deserialing is case-insensitive.

Fixes: clap-rs#88
@epage
Copy link
Member

epage commented Sep 26, 2024

#114 works to adopt words for the serialization format.

It started by delegating to LogLevels serde support but there wasn't a way to distinguish "off" from "unset" / "default".

We tried switching to LevelFilter but all values are serialized in SCREMAING_CASE (though snake_case is accepted for deserialization apparently). We are stuck between being consistent with LevelFilter and being consistent with the style of most serialized values.

joshka added a commit to joshka/clap-verbosity-flag that referenced this issue Sep 26, 2024
This commit adds serialization and deserialization support for the
Verbosity type. The verbosity is serialized using the log::LevelFilter
enum that represents the equivalent number of verbose and quiet flags.

The serialized value is the uppercase variant of the enum variant.
Deserialing is case-insensitive.

Fixes: clap-rs#88
@joshka
Copy link
Contributor

joshka commented Sep 26, 2024

We tried switching to LevelFilter but all values are serialized in SCREMAING_CASE (though snake_case is accepted for deserialization apparently). We are stuck between being consistent with LevelFilter and being consistent with the style of most serialized values.

I think this is a reasonable tradeoff to make rather than manually implementing serialize. My thinking is that it's consistent with how log::Level or log::LevelFilter would be serialized if that was directly used instead of the verbosity flag being interjected. It's a two way door if that's a wrong assumption, as it's compatible to change the serialization format in the future, so long as deserialization still works.

I'm also fine with changing this to lowercase, but I'm 90/10 that uppercase is the right choice though.

joshka added a commit to joshka/clap-verbosity-flag that referenced this issue Sep 26, 2024
This commit adds serialization and deserialization support for the
Verbosity type. The verbosity is serialized using the log::LevelFilter
enum that represents the equivalent number of verbose and quiet flags.

The serialized value is the uppercase variant of the enum variant.
Deserialing is case-insensitive.

Fixes: clap-rs#88
joshka added a commit to joshka/clap-verbosity-flag that referenced this issue Nov 29, 2024
Verbosity is serialized and deserialized using the title case of the
VerbosityFilter (e.g. "Debug")

The `serde` dependency is gated behind an optional feature flag.

Added conversion methods between Verbosity and VerbosityFilter to
simplify the implementation and derived PartialEq, Eq impls on
types where this was necesary for testing.

Fixes: clap-rs#88
joshka added a commit to joshka/clap-verbosity-flag that referenced this issue Nov 29, 2024
Verbosity is serialized and deserialized using the title case of the
VerbosityFilter (e.g. "Debug")

The `serde` dependency is gated behind an optional feature flag.

Added conversion methods between Verbosity and VerbosityFilter to
simplify the implementation and derived PartialEq, Eq impls on
types where this was necesary for testing.

Fixes: clap-rs#88
epage pushed a commit to joshka/clap-verbosity-flag that referenced this issue Dec 6, 2024
Verbosity is serialized and deserialized using the title case of the
VerbosityFilter (e.g. "Debug")

The `serde` dependency is gated behind an optional feature flag.

Added conversion methods between Verbosity and VerbosityFilter to
simplify the implementation and derived PartialEq, Eq impls on
types where this was necesary for testing.

Fixes: clap-rs#88
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants