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

sim-cli: remove broken parser for activity multiplier #155

Merged
merged 1 commit into from
Oct 31, 2023

Conversation

carlaKC
Copy link
Contributor

@carlaKC carlaKC commented Oct 31, 2023

u64 parser does not work for a f64 value.

Broken on main:

sim-cli sim.json
thread 'main' panicked at 'Mismatch between definition and access of `capacity_multiplier`. Could not downcast to TypeId { t: 8711759054683223602271599665973969982 }, need to downcast to TypeId { t: 25882202575019293479932656973818029271 }
', /Users/carla/.cargo/registry/src/index.crates.io-6f17d22bba15001f/clap_builder-4.4.6/src/parser/error.rs:32:9
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
sim-cli sim.json --capacity-multiplier 0.5
error: invalid value '0.5' for '--capacity-multiplier <CAPACITY_MULTIPLIER>': invalid digit found in string

Copy link
Collaborator

@okjodom okjodom left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm. we could replace this with a custom value parser

@carlaKC carlaKC merged commit 6ac6d9a into bitcoin-dev-project:main Oct 31, 2023
@sr-gi
Copy link
Member

sr-gi commented Oct 31, 2023

Oh damn, my bad :(

We can use a StringValueParser with try_map to fix this.

#[clap(long, short, default_value_t = ACTIVITY_MULTIPLIER, value_parser = clap::builder::StringValueParser::new().try_map(|x| {
    match x.parse::<f64>() {
        Ok(x) => {
            if x > 0.0 {
                Ok(x)
            } else {
                Err(format!("capacity_multiplier must be higher than 0. {x} received."))
            }
        },
        Err(e) =>  {
            Err(e.to_string())
        }
    }   
}))]

Notice this does not have to be inlined, we can define a function elsewhere and jut pass it to try_map

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 this pull request may close these issues.

3 participants