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

allow into duration to take an integer amount of ns #10286

Merged
merged 3 commits into from Sep 9, 2023

Conversation

amtoine
Copy link
Member

@amtoine amtoine commented Sep 9, 2023

related to

Description

because 1_234 | into datetime takes an integer number of ns and 1_234 | into filesize takes an integer amount of bytes, i think 1_234 | into duration should also be valid and see 1_234 as an integer amount of ns ๐Ÿ˜‹

User-Facing Changes

before

either

1234 | into string | $in ++ "ns" | into duration
1234 | $"($in)ns" | into duration

or

1234 * 1ns

and

> 1_234 | into duration
Error: nu::parser::input_type_mismatch

  ร— Command does not support int input.
   โ•ญโ”€[entry #2:1:1]
 1 โ”‚ 1_234 | into duration
   ยท         โ”€โ”€โ”€โ”€โ”€โ”€โ”ฌโ”€โ”€โ”€โ”€โ”€โ”€
   ยท               โ•ฐโ”€โ”€ command doesn't support int input
   โ•ฐโ”€โ”€โ”€โ”€

after

> 1_234 | into duration
1ยตs 234ns

Tests + Formatting

new example test

Example {
    description: "Convert a number of ns to duration",
    example: "1_234_567 | into duration",
    result: Some(Value::duration(1_234_567, span)),
}

After Submitting

@amtoine
Copy link
Member Author

amtoine commented Sep 9, 2023

i wanted to add a --unit: string option to into duration, tried to get the option with

    let unit: String = call
        .get_flag::<String>(engine_state, stack, "unit")
        .unwrap()
        .unwrap_or("ns".into())
        .clone();

but keep getting the following error

error[E0507]: cannot move out of `unit`, a captured variable in an `FnMut` closure
   --> crates/nu-command/src/conversions/into/duration.rs:152:28
    |
143 |     let unit: String = call
    |         ---- captured outer variable
...
150 |         move |v| {
    |         -------- captured by this `FnMut` closure
151 |             if column_paths.is_empty() {
152 |                 action(&v, unit, span)
    |                            ^^^^ move occurs because `unit` has type `std::string::String`, which does not implement the `Copy` trait

@fdncred
Copy link
Collaborator

fdncred commented Sep 9, 2023

I wanted to add a --unit: int

I don't understand why --unit would be an int

@amtoine
Copy link
Member Author

amtoine commented Sep 9, 2023

woopsie @fdncred
i wanted to say a string, e.g. ms or sec ๐Ÿ˜Œ

@fdncred
Copy link
Collaborator

fdncred commented Sep 9, 2023

Maybe this will help @amtoine? I figured some of it out I think.

Add to the signature

            .named(
                "unit",
                SyntaxShape::String,
                "Unit to convert number into",
                Some('u'),
            )

Parse the unit

    let unit = match call.get_flag::<String>(engine_state, stack, "unit")? {
        Some(sep) => {
            if ["ns", "us", "ยตs", "ms", "sec", "min", "hr", "day", "wk"]
                .iter()
                .any(|d| d == &sep)
            {
                sep
            } else {
                return Err(ShellError::CantConvertToDuration {
                    details: sep,
                    dst_span: span,
                    src_span: span,
                    help: Some(
                        "supported units are ns, us/ยตs, ms, sec, min, hr, day, and wk".to_string(),
                    ),
                });
            }
        }
        None => "ns".to_string(),
    };

Use it in the map

    input.map(
        move |v| {
            if column_paths.is_empty() {
                action(&v, &unit.clone(), span)
            } else {
                let unitclone = &unit.clone();
                let mut ret = v;
                for path in &column_paths {
                    let r = ret.update_cell_path(
                        &path.members,
                        Box::new(move |old| action(old, &unitclone, span)),
                    );
                    if let Err(error) = r {
                        return Value::error(error, span);
                    }
                }

                ret
            }
        },
        engine_state.ctrlc.clone(),
    )

Change the action signature

fn action(input: &Value, unit: &str, span: Span) -> Value {

@amtoine
Copy link
Member Author

amtoine commented Sep 9, 2023

mm so you use a cloned binding in the move block, interesting ๐Ÿ˜

@amtoine
Copy link
Member Author

amtoine commented Sep 9, 2023

pretty cool now, right? ๐Ÿ˜

> random dice --dice 4 --sides 1_000_000_000_000 | each { into duration }
โ•ญโ”€โ”€โ”€โ”ฌโ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ•ฎ
โ”‚ 0 โ”‚ 9min 43sec 287ms 758ยตs 891ns โ”‚
โ”‚ 1 โ”‚ 3min 21sec 594ms 789ยตs 374ns โ”‚
โ”‚ 2 โ”‚ 5min 11sec 313ms 674ยตs 663ns โ”‚
โ”‚ 3 โ”‚ 4min 36sec 708ms 968ยตs 586ns โ”‚
โ•ฐโ”€โ”€โ”€โ”ดโ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ•ฏ

@fdncred
Copy link
Collaborator

fdncred commented Sep 9, 2023

mm so you use a cloned binding in the move block, interesting ๐Ÿ˜

And made action take a &str instead of a String. I tried to make unit a &str too but it needed a lifetime, which is just a pain.

I tested it, looks good to me.

@fdncred fdncred merged commit 17abbdf into nushell:main Sep 9, 2023
19 checks passed
@fdncred
Copy link
Collaborator

fdncred commented Sep 9, 2023

thanks

@amtoine amtoine deleted the int-into-duration branch September 10, 2023 08:46
@sholderbach sholderbach added the pr:release-note-mention Addition/Improvement to be mentioned in the release notes label Sep 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr:release-note-mention Addition/Improvement to be mentioned in the release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants