Skip to content

feat: duration from record#15600

Merged
fdncred merged 15 commits intonushell:mainfrom
LoicRiegel:duration-from-record
Apr 19, 2025
Merged

feat: duration from record#15600
fdncred merged 15 commits intonushell:mainfrom
LoicRiegel:duration-from-record

Conversation

@LoicRiegel
Copy link
Copy Markdown
Contributor

Closes #15543

Description

  1. Simplify code in datetime.rs based on a suggestion in my last PR on "datetime from record"
  2. Make into duration work with durations inside a record, provided as a cell path
  3. Make into duration work with durations as record

User-Facing Changes

# Happy paths
~> {d: '1hr'} | into duration d
╭───┬─────╮
 d  1hr 
╰───┴─────╯

~> {week: 10, day: 2, sign: '+'} | into duration
10wk 2day

# Error paths and invalid usage
~> {week: 10, day: 2, sign: 'x'} | into duration
Error: nu::shell::incorrect_value

  × Incorrect value.
   ╭─[entry #4:1:26]
 1  {week: 10, day: 2, sign: 'x'} | into duration
   ·                          ─┬─    ──────┬──────
   ·                                      ╰── encountered here
   ·                           ╰── Invalid sign. Allowed signs are +, -
   ╰────

~> {week: 10, day: -2, sign: '+'} | into duration
Error: nu::shell::incorrect_value

  × Incorrect value.
   ╭─[entry #5:1:17]
 1  {week: 10, day: -2, sign: '+'} | into duration
   ·                 ─┬               ──────┬──────
   ·                                       ╰── encountered here
   ·                  ╰── number should be positive
   ╰────

~> {week: 10, day: '2', sign: '+'} | into duration
Error: nu::shell::only_supports_this_input_type

  × Input type not supported.
   ╭─[entry #6:1:17]
 1  {week: 10, day: '2', sign: '+'} | into duration
   ·                 ─┬─               ──────┬──────
   ·                                        ╰── only int input data is supported
   ·                  ╰── input type: string
   ╰────

~> {week: 10, unknown: 1} | into duration
Error: nu::shell::unsupported_input

  × Unsupported input
   ╭─[entry #7:1:1]
 1  {week: 10, unknown: 1} | into duration
   · ───────────┬──────────   ──────┬──────
   ·                               ╰── Column 'unknown' is not valid for a structured duration. Allowed columns are: week, day, hour, minute, second, millisecond, microsecond, nanosecond, sign
   ·            ╰── value originates from here
   ╰────

~> {week: 10, day: 2, sign: '+'} | into duration --unit sec
Error: nu::shell::incompatible_parameters

  × Incompatible parameters.
   ╭─[entry #2:1:33]
 1  {week: 10, day: 2, sign: '+'} | into duration --unit sec
   ·                                 ──────┬────── ─────┬────
   ·                                                   ╰── the units should be included in the record
   ·                                       ╰── got a record as input
   ╰────

Tests + Formatting

  • Add examples and integration tests for into duration
  • Add one test for into duration

After Submitting

If this is merged in time, I'll update my PR on the "datetime handling highlights" for the release notes.

@fdncred
Copy link
Copy Markdown
Contributor

fdncred commented Apr 18, 2025

Cool! This looks interesting.

What's going on here?

{week: 10, day: 2, sign: '+'} | into duration --unit sec
Error: nu::shell::incompatible_parameters

  × Incompatible parameters.
   ╭─[entry #2:1:33]
 1  {week: 10, day: 2, sign: '+'} | into duration --unit sec
   ·                                 ──────┬────── ─────┬────
   ·                                                   ╰── the units should be included in the record
   ·                                       ╰── got a record as input

I can do this, but maybe I shouldn't be able to?

 10wk + 2day | into duration --unit sec
10wk 2day

@LoicRiegel
Copy link
Copy Markdown
Contributor Author

Ah, yeah interesting, that shows inconsistent behavior. I implemented {week: 10, day: 2, sign: '+'} | into duration --unit sec to throw an error because it does not make sense to say that a record is in a particular unit. It makes sense when the input is a number, e.g. 1 | into duration --unit sec, but when you have a record as input, the units are in the columns, so...
I also did that previously with the into datetime command, if you pass a record as input, you'll get an error if you also provide a --format or --timezone flag.

But with the second command from you previous comment, I think it also doesn't make sense to be able to pass a unit when you already have a duration as input, but here it look slike the --unit flag is just purely ignored.

I like consistency, but I have the intuition that the "ignoring" behavior is currently more common across the differnt commands, so if we wanted to change that, this could take some time. On the other hand, I feel like getting an error in those cases really does make sense...
What do you think?

Well, maybe we could also say that having this slight inconsistent behavior is not our highest priority to fix 😅

@fdncred
Copy link
Copy Markdown
Contributor

fdncred commented Apr 18, 2025

I agree that it's for 1 | into duration --unit sec and the other scenarios should probably fail.

@LoicRiegel
Copy link
Copy Markdown
Contributor Author

Done 🟢 --unit flag can now only be used with string, int and float inputs, and is not allowed with record and duration inputs.

@LoicRiegel LoicRiegel force-pushed the duration-from-record branch 2 times, most recently from dc80ffb to cc17390 Compare April 19, 2025 20:54
@fdncred fdncred merged commit 5c59611 into nushell:main Apr 19, 2025
15 checks passed
@fdncred
Copy link
Copy Markdown
Contributor

fdncred commented Apr 19, 2025

Thanks

@github-actions github-actions bot added this to the v0.104.0 milestone Apr 19, 2025
@LoicRiegel
Copy link
Copy Markdown
Contributor Author

I'll update my PR on the release notes' branch to mention this as well

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.

Construct duration from record

2 participants