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

Fix inconsistent print behavior #12675

Merged
merged 1 commit into from
Apr 27, 2024

Conversation

devyn
Copy link
Contributor

@devyn devyn commented Apr 26, 2024

Description

I found a bunch of issues relating to the specialized reimplementation of print() that's done in nu-cli and it just didn't seem necessary. So I tried to unify the behavior reasonably. PipelineData::print() already handles the call to table and it even has a no_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:

Screenshot_2024-04-26_02-02-29

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 target BufferedReader, 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, because lines 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

  • Breaking: BufferedReader changes + no more public fields
  • A raw I/O stream from e.g. open won't produce infinite errors anymore, but I consider that to be a plus
  • the implicit print on script output is the same as the normal one now

Tests + Formatting

Everything passes but I didn't add anything specific.

@fdncred
Copy link
Collaborator

fdncred commented Apr 26, 2024

I'm wondering if this is the problem that I see from time to time that occurs at exactly 8192 bytes. I was using nushell in my job one time and it kept messing up the data. IIRC, it was truncating at 8192, throwing away the rest of the line, and then continuing which made for messed up data. Sophia helped me work around it by using collect, but the problem that I mention still existed after that work-around.

Copy link
Member

@IanManske IanManske left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, this looks good. I'm good with landing this now or after 0.93.0.

Also, @fdncred I plan to tackle the BufReader 8192 bytes issue after 0.93.0.

@fdncred
Copy link
Collaborator

fdncred commented Apr 26, 2024

Sounds good @IanManske. Thanks for the heads-up. I'll be watching for it.

@IanManske IanManske merged commit 02de69d into nushell:main Apr 27, 2024
15 checks passed
@hustcer hustcer added this to the v0.93.0 milestone Apr 27, 2024
@devyn devyn deleted the fix-inconsistent-print-behavior branch April 27, 2024 07:00
maxim-uvarov pushed a commit to maxim-uvarov/nushell that referenced this pull request May 1, 2024
# Description

I found a bunch of issues relating to the specialized reimplementation
of `print()` that's done in `nu-cli` and it just didn't seem necessary.
So I tried to unify the behavior reasonably. `PipelineData::print()`
already handles the call to `table` and it even has a `no_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:


![Screenshot_2024-04-26_02-02-29](https://github.com/nushell/nushell/assets/10729/131c2224-08ee-4582-8617-6ecbb3ce8da5)

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 target `BufferedReader`, 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`, because `lines`
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
- **Breaking**: `BufferedReader` changes + no more public fields
- A raw I/O stream from e.g. `open` won't produce infinite errors
anymore, but I consider that to be a plus
- the implicit `print` on script output is the same as the normal one
now

# Tests + Formatting
Everything passes but I didn't add anything specific.
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.

None yet

4 participants