Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
I found a bunch of issues relating to the specialized reimplementation of
print()
that's done innu-cli
and it just didn't seem necessary. So I tried to unify the behavior reasonably.PipelineData::print()
already handles the call totable
and it even has ano_newline
option.One of the most major issues before was that we were using the value iterator, and then converting to string, and then printing each with newlines. This doesn't work well for an external stream, because its iterator ends up creating
Value::binary()
with each buffer... so we were doing lossy UTF-8 conversion on those and then printing them with newlines, which was very weird:You can see the random newline inserted in a break between buffers, but this would be even worse if it were on a multibyte UTF-8 character. You can produce this by writing a large amount of text to a text file, and then doing
nu -c 'open file.txt'
- in my case I just wrote^find .
; it just has to be large enough to trigger a buffer break.Using
print()
instead led to a new issue though, because it doesn't abort on errors. This is so that certain commands can produce a stream of errors and have those all printed. There are tests for e.g.rm
that depend on this behavior. I assume we want to keep that, so instead I made my targetBufferedReader
, and had that fuse closed if an error was encountered. I can't imagine we want to keep reading from a wrapped I/O stream if an error occurs; more often than not the error isn't going to magically resolve itself, it's not going to be a different error each time, and it's just going to lead to an infinite stream of the same error.The test that broke without that was
open . | lines
, becauselines
doesn't fuse closed on error. But I don't know if it's expected or not for it to do that, so I didn't target that.I think this PR makes things better but I'll keep looking for ways to improve on how errors and streams interact, especially trying to eliminate cases where infinite error loops can happen.
User-Facing Changes
BufferedReader
changes + no more public fieldsopen
won't produce infinite errors anymore, but I consider that to be a plusprint
on script output is the same as the normal one nowTests + Formatting
Everything passes but I didn't add anything specific.