Add string/binary type color to `ByteStream` #12897

devyn · 2024-05-17T15:54:42Z

Description

This PR allows byte streams to optionally be colored as being specifically binary or string data, which guarantees that they'll be converted to Binary or String appropriately on into_value(), making them compatible with Type guarantees. This makes them significantly more broadly usable for command input and output.

There is still an Unknown type for byte streams coming from external commands, which uses the same behavior as we previously did where it's a string if it's UTF-8.

A small number of commands were updated to take advantage of this, just to prove the point. I will be adding more after this merges.

User-Facing Changes

New types in describe: string (stream), binary (stream)
These commands now return a stream if their input was a stream:
- into binary
- into string
- bytes collect
- str join
- first (binary)
- last (binary)
- take (binary)
- skip (binary)
Streams that are explicitly binary colored will print as a streaming hexdump
- example:
```
1.. | each { into binary } | bytes collect
```

Tests + Formatting

I've added some tests to cover it at a basic level, and it doesn't break anything existing, but I do think more would be nice. Some of those will come when I modify more commands to stream.

After Submitting

There are a few things I'm not quite satisfied with:

String trimming behavior. We automatically trim newlines from streams from external commands, but I don't think we should do this with internal commands. If I call a command that happens to turn my string into a stream, I don't want the newline to suddenly disappear. I changed this to specifically do it only on Child and File, but I don't know if this is quite right, and maybe we should bring back the old flag for trim_end_newline
Known binary always resulting in a hexdump. It would be nice to have a print --raw, so that we can put binary data on stdout explicitly if we want to. This PR doesn't change how external commands work though - they still dump straight to stdout.

Otherwise, here's the normal checklist:

release notes
docs update for plugin protocol changes (added type field)

squashed commits: ---- Be more strict about stream type, and always require it ---- make type non-Option, because None doesn't look very clear ---- also clean up tee a bunch ---- Add helpers for mapping between ByteStream and ListStream ---- do some cleanup and rethink how to map PipelineData into ByteStream ---- make describe output for byte streams mirror that of list streams ---- Temporary fix: print binary streams by collecting them through pretty_hex() first ---- fix `describe -n` on byte streams to not drain ---- streaming pretty hex output for binary streams ---- Don't pipe byte streams through `table` when passing to external commands ---- remove extraneous write ---- Replace the `VecDeque` with a `Cursor<Vec<u8>>` Since the vec is always empty when generating anyway, there's no need for a `VecDeque`. That would be better if we wanted to write stuff while there is still data. The `Cursor<Vec<u8>>` is less complex in implementation and should be a bit faster ---- preserve stream for `into binary`, `into string` ---- make sure `PipelineData::map` respects byte stream type ---- Make the Values iterator buffering more sensible Will return whatever buffers it gets, tries to assemble correct UTF-8 for strings. No line-based buffering here, use `lines` for that if you need it ---- fix `lines`, and change `Lines` to return strings without line ending ---- fix something broken by the merge ---- don't need to take line endings off in Lines because ByteLines already does it fixup 1

test str join

fix `join`: iter should not have been strict fixup 2 test bytes collect

crates/nu-protocol/src/pipeline/pipeline_data.rs

crates/nu-protocol/src/pipeline/byte_stream.rs

crates/nu-engine/src/command_prelude.rs

crates/nu-protocol/src/io/buffers.rs

crates/nu-protocol/src/io/values/mod.rs

crates/nu-protocol/src/pipeline/byte_stream.rs

crates/nu-command/src/viewers/table.rs

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

crates/nu-protocol/src/pipeline/byte_stream.rs

IanManske · 2024-05-17T19:52:37Z

Thanks @devyn, it looks good for the most part. Looks like there's some merge/rebase artifacts, sorry for all the force pushes lol.

fdncred · 2024-05-18T01:10:49Z

Does this not work just because all the commands haven't been ported yet?

❯ open README.md | into binary | first 100
Error: nu::shell::only_supports_this_input_type

  × Input type not supported.
   ╭─[entry #2:1:1]
 1 │ open README.md | into binary | first 100
   · ──┬─                           ──┬──
   ·   │                              ╰── only list, binary or range input data is supported
   ·   ╰── input type: byte stream
   ╰────

Co-authored-by: Ian Manske <ian.manske@pm.me>

devyn · 2024-05-18T05:27:01Z

No worries Ian.

@fdncred - since a ByteStream should behave the same way, I think I should fix that here. I don't want to cause anything to break in this PR, it should really just make things stream but otherwise work the same way they did before

Co-authored-by: Ian Manske <ian.manske@pm.me>

fdncred · 2024-05-18T13:20:57Z

@fdncred - since a ByteStream should behave the same way, I think I should fix that here. I don't want to cause anything to break in this PR, it should really just make things stream but otherwise work the same way they did before

sounds good to me. I just use first, last, skip, take quite a bit with binaries and into binary so I'd hope it would keep working.

crates/nu-command/src/strings/str_/join.rs

crates/nu-protocol/src/pipeline/byte_stream.rs

devyn · 2024-05-19T16:29:58Z

@fdncred

sounds good to me. I just use first, last, skip, take quite a bit with binaries and into binary so I'd hope it would keep working.

so I got first, last, skip, take all working. I also found that skip functionally supported binary even though its type doesn't actually allow for that, so I added that & an example. You'd have to have been using any before for it to work

Anyway it should all be consistent now

devyn · 2024-05-19T16:32:49Z

@IanManske you mentioned tee before but I didn't quite get to look at what you meant. I had refactored it further and possibly some of the code was old - is it missing anything?

IanManske · 2024-05-19T18:19:50Z

you mentioned tee before but I didn't quite get to look at what you meant.

I'm not sure either lol, I must have read the code wrong. Anyways, the tee looks much nicer now, thanks!

devyn added 5 commits May 17, 2024 08:24

streaming: str join

c4d4081

test str join

streaming: bytes collect

80a5a1c

fix `join`: iter should not have been strict fixup 2 test bytes collect

replace r#type with type_

38fba51

fix accidentally added collect in the wrong place

d6c710d

devyn requested a review from IanManske May 17, 2024 16:15

IanManske reviewed May 17, 2024

View reviewed changes

IanManske added pr:plugins This PR is related to plugins pr:breaking-change This PR implies a change affecting users and has to be noted in the release notes pr:commands This PR changes our commands in some way labels May 17, 2024

Update crates/nu-command/src/viewers/table.rs

ba78351

Co-authored-by: Ian Manske <ian.manske@pm.me>

devyn and others added 5 commits May 17, 2024 22:32

Update crates/nu-protocol/src/pipeline/byte_stream.rs

06aad79

Co-authored-by: Ian Manske <ian.manske@pm.me>

Update crates/nu-protocol/src/pipeline/pipeline_data.rs

a0dea92

Co-authored-by: Ian Manske <ian.manske@pm.me>

Update crates/nu-command/src/viewers/table.rs

0ca8c86

Co-authored-by: Ian Manske <ian.manske@pm.me>

Correct the Chunks implementation with the new type-aware one

ee3b208

Merge branch 'main' into bytestream-streaming-commands

5197d2c

IanManske reviewed May 18, 2024

View reviewed changes

crates/nu-command/src/strings/str_/join.rs Show resolved Hide resolved

crates/nu-protocol/src/pipeline/byte_stream.rs Outdated Show resolved Hide resolved

crates/nu-protocol/src/pipeline/byte_stream.rs Outdated Show resolved Hide resolved

devyn added 8 commits May 19, 2024 08:17

bring a removed comment back

e18a8f5

truncate(0) -> clear()

72e275f

Trim on files, in addition to child processes

42d8d08

extra comma

7694f50

replace bytestream type messages with a describe() helper

6f87887

Add bytestream support to first, last, take, skip

a7c5144

Merge branch 'main' into bytestream-streaming-commands

a5da549

oops fix something with the merge

10a5402

IanManske approved these changes May 19, 2024

View reviewed changes

IanManske merged commit c61075e into nushell:main May 20, 2024
15 checks passed

hustcer added this to the v0.94.0 milestone May 20, 2024

devyn deleted the bytestream-streaming-commands branch May 20, 2024 05:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add string/binary type color to `ByteStream` #12897

Add string/binary type color to `ByteStream` #12897

devyn commented May 17, 2024 •

edited

IanManske commented May 17, 2024 •

edited

fdncred commented May 18, 2024

devyn commented May 18, 2024

fdncred commented May 18, 2024

devyn commented May 19, 2024

devyn commented May 19, 2024

IanManske commented May 19, 2024 •

edited

Add string/binary type color to ByteStream #12897

Add string/binary type color to ByteStream #12897

Conversation

devyn commented May 17, 2024 • edited

Description

User-Facing Changes

Tests + Formatting

After Submitting

IanManske commented May 17, 2024 • edited

fdncred commented May 18, 2024

devyn commented May 18, 2024

fdncred commented May 18, 2024

devyn commented May 19, 2024

devyn commented May 19, 2024

IanManske commented May 19, 2024 • edited

Add string/binary type color to `ByteStream` #12897

Add string/binary type color to `ByteStream` #12897

devyn commented May 17, 2024 •

edited

IanManske commented May 17, 2024 •

edited

IanManske commented May 19, 2024 •

edited