Update description and error types for `split-by` by drbrain · Pull Request #10865 · nushell/nushell

drbrain · 2023-10-28T19:47:30Z

Description

split-by only works on a Record, the error type was updated to match, and now uses a more-specific type. (Two type fixes for the price of one!)

The usage was updated to say "record" as well

User-Facing Changes

Providing the wrong type to split-by now gives an error messages with the correct required input type

Previously:

❯ ls | get name | split-by type
Error:   × unsupported input
   ╭─[entry #267:1:1]
 1 │ ls | get name | split-by type
   ·      ─┬─
   ·       ╰── requires a table with one row for splitting
   ╰────

With this PR:

❯ ls | get name | split-by type
Error: nu::shell::type_mismatch

  × Type mismatch.
   ╭─[entry #1:1:1]
 1 │ ls | get name | split-by type
   ·      ─┬─
   ·       ╰── requires a record to split
   ╰────

Tests + Formatting

🟢 toolkit fmt
🟢 toolkit clippy
🟢 toolkit test
🟢 toolkit test stdlib

After Submitting

Only generated commands need to be updated

`split-by` only works on a `Record`, the error was updated to match, and now uses a more-specific type The `usage` was updated to say "record" as well

sholderbach

Thanks for improving the messages here!

crates/nu-command/src/filters/split_by.rs

fdncred · 2023-10-28T21:22:47Z

Wait, split-by doesn't work on tables? I thought all the blah-by commands did?

drbrain · 2023-10-28T22:03:12Z

Wait, split-by doesn't work on tables? I thought all the blah-by commands did?

❯ nu --version
0.86.0
❯ ls | split-by type
Error: nu::parser::input_type_mismatch

  × Command does not support table input.
   ╭─[entry #4:1:1]
 1 │ ls | split-by type
   ·      ────┬───
   ·          ╰── command doesn't support table input
   ╰────

❯ ls | describe
table<name: string, type: string, size: filesize, modified: date> (stream)

PS: I'm unclear on what the difference between group-by and split-by in terms of functionality is supposed to be, they seem to be sibling commands that maybe could be merged into one?

fdncred · 2023-10-28T22:41:14Z

ya, I've never used split-by. By the example it has, it looks like it reads into nested data.

> {
        '2019': [
          { name: 'andres', lang: 'rb', year: '2019' },
          { name: 'jt', lang: 'rs', year: '2019' }
        ],
        '2021': [
          { name: 'storm', lang: 'rs', 'year': '2021' }
        ]
    } | split-by lang
  ╭──┬─────────────────╮
  │rb│{record 1 field} │
  │rs│{record 2 fields}│
  ╰──┴─────────────────╯

There are other *-by commands though. group-by, split-by, sort-by, uniq-by. I figured they'd all work on tables, but I guess not. Seems like split-by is a bit niche.

amtoine · 2023-10-29T12:15:12Z

sounds good 👌
if we can get the review threads to close, i'm all for this better error 🙏

fdncred · 2023-10-29T13:32:58Z

Looking at the example some more after playing around with input_output_types() in this command, I'm wondering if the only way this command works is if it's a record with tables as values?

I'd love to see more examples in the help of how this command works.

fdncred

Thanks for working on this error.

sholderbach · 2023-10-29T21:09:20Z

crates/nu-command/src/filters/split_by.rs

-                Vec::new(),
-            ))
-        }
+        _ => return Err(ShellError::PipelineEmpty { dst_span }),


If I read the code leading to this point correctly this could still be more than PipelineData::Empty e.g. an ExternalStream, right?

Yes. In 84661a5 I split PipelineData::Empty out from the other types which return a PipelineMismatch. I looked around and this seems most appropriate as merge treats a ListStream as a List and ExternalStream currently can't produce a Record.

crates/nu-command/tests/commands/split_by.rs

fdncred · 2023-11-03T14:27:19Z

@drbrain Are you still looking into this?

Neither of these pipeline types produce a single Record which is all split-by can handle.

drbrain · 2023-11-04T06:26:51Z

I had a busy week.

It should be ready to go with 84661a5

sholderbach

Awesome thanks, this looks good and well covered

# Description `split-by` only works on a `Record`, the error type was updated to match, and now uses a more-specific type. (Two type fixes for the price of one!) The `usage` was updated to say "record" as well # User-Facing Changes * Providing the wrong type to `split-by` now gives an error messages with the correct required input type Previously: ``` ❯ ls | get name | split-by type Error: × unsupported input ╭─[entry nushell#267:1:1] 1 │ ls | get name | split-by type · ─┬─ · ╰── requires a table with one row for splitting ╰──── ``` With this PR: ``` ❯ ls | get name | split-by type Error: nu:🐚:type_mismatch × Type mismatch. ╭─[entry #1:1:1] 1 │ ls | get name | split-by type · ─┬─ · ╰── requires a record to split ╰──── ``` # Tests + Formatting - 🟢 `toolkit fmt` - 🟢 `toolkit clippy` - 🟢 `toolkit test` - 🟢 `toolkit test stdlib` # After Submitting Only generated commands need to be updated --------- Co-authored-by: Darren Schroeder <343840+fdncred@users.noreply.github.com>

# Description `split-by` only works on a `Record`, the error type was updated to match, and now uses a more-specific type. (Two type fixes for the price of one!) The `usage` was updated to say "record" as well # User-Facing Changes * Providing the wrong type to `split-by` now gives an error messages with the correct required input type Previously: ``` ❯ ls | get name | split-by type Error: × unsupported input ╭─[entry nushell#267:1:1] 1 │ ls | get name | split-by type · ─┬─ · ╰── requires a table with one row for splitting ╰──── ``` With this PR: ``` ❯ ls | get name | split-by type Error: nu:🐚:type_mismatch × Type mismatch. ╭─[entry nushell#1:1:1] 1 │ ls | get name | split-by type · ─┬─ · ╰── requires a record to split ╰──── ``` # Tests + Formatting - 🟢 `toolkit fmt` - 🟢 `toolkit clippy` - 🟢 `toolkit test` - 🟢 `toolkit test stdlib` # After Submitting Only generated commands need to be updated --------- Co-authored-by: Darren Schroeder <343840+fdncred@users.noreply.github.com>

Update description and error messages for split-by

be9327e

`split-by` only works on a `Record`, the error was updated to match, and now uses a more-specific type The `usage` was updated to say "record" as well

sholderbach reviewed Oct 28, 2023

View reviewed changes

crates/nu-command/src/filters/split_by.rs Outdated Show resolved Hide resolved

Use OnlySupportsThisInputType and PipelineEmpty errors in split-by

695a578

drbrain requested a review from sholderbach October 28, 2023 23:45

fdncred approved these changes Oct 29, 2023

View reviewed changes

tweak the example a little

e1e2225

sholderbach reviewed Oct 29, 2023

View reviewed changes

Return PipelineMismatch from split-by for ListStream and ExternalStream

84661a5

Neither of these pipeline types produce a single Record which is all split-by can handle.

drbrain requested a review from sholderbach November 6, 2023 23:13

sholderbach approved these changes Nov 7, 2023

View reviewed changes

sholderbach merged commit c039e4b into nushell:main Nov 7, 2023

Conversation

drbrain commented Oct 28, 2023

Description

User-Facing Changes

Tests + Formatting

After Submitting

Uh oh!

sholderbach left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

fdncred commented Oct 28, 2023

Uh oh!

drbrain commented Oct 28, 2023

Uh oh!

fdncred commented Oct 28, 2023

Uh oh!

amtoine commented Oct 29, 2023

Uh oh!

fdncred commented Oct 29, 2023

Uh oh!

fdncred left a comment

Choose a reason for hiding this comment

Uh oh!

sholderbach Oct 29, 2023

Choose a reason for hiding this comment

Uh oh!

drbrain Nov 4, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

fdncred commented Nov 3, 2023

Uh oh!

drbrain commented Nov 4, 2023

Uh oh!

sholderbach left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants