make save stream on list stream data#7675
Conversation
sholderbach
left a comment
There was a problem hiding this comment.
Not sure if this would lead to a regression how the output gets formatted.
| file.write_all(&value_to_bytes(val)?) | ||
| .map_err(|err| ShellError::IOError(err.to_string()))?; | ||
| file.write_all("\n".as_bytes()) | ||
| .map_err(|err| ShellError::IOError(err.to_string()))?; |
There was a problem hiding this comment.
Not sure if this is the correct behavior if we want to generate a file in a streaming way for some file types?
This change would always use the Value::as_string() if I understand it. But formats like nuon/csv/json are reasonable to stream from a list of values, with some effort.
There was a problem hiding this comment.
Yeah, this is a regression. Previously ls | save foo.json converted ls results to JSON, now it fails with Can't convert to string.
Maybe that should be fixed. But to be honest I don't like the existing functionality where save magically converts formats based on the file extension, and I would not be sad if someone decided to get rid of it.
There was a problem hiding this comment.
But formats like nuon/csv/json are reasonable to stream from a list of values, with some effort.
Ah, that's a good catch.
Maybe that should be fixed. But to be honest I don't like the existing functionality where save magically converts formats based on the file extension, and I would not be sad if someone decided to get rid of it.
I think we should keep this behavior, we have --raw flag to disable it
There was a problem hiding this comment.
Currently it will only stream data if --raw flag is provided or target file doesn't have an extension
There was a problem hiding this comment.
Sorry not sure if I consider this resolved.
There was a problem hiding this comment.
@sholderbach My bad, sorry for merging.
What exactly are you looking for? ls | save foo.json now converts to JSON as before.
There was a problem hiding this comment.
Yeah, I think for something like ls | save foo.json, the data is converted to json, streaming only happened when we provide --raw flag
| other => Err(ShellError::OnlySupportsThisInputType( | ||
| "string, binary or list".into(), | ||
| other.get_type().to_string(), | ||
| span, | ||
| // This line requires the Value::Error match above. | ||
| other.expect_span(), | ||
| )), | ||
| other => Ok(other.as_string()?.into_bytes()), |
There was a problem hiding this comment.
What are the intended semantics here? Was this a recent regression or is this making things more implicit?
There was a problem hiding this comment.
Previously, when calling input.into_value(span), if the input is PipelineData::ListStream, the value will be Value::List, then value_to_bytes will try to map each element as string.
It will goes to error if we're trying to iterate through ListStream and doesn't try to convert from element(maybe integer) to string
|
Thank you for working on this! I'm always really happy to see streaming improvements.
Yeah, it's difficult to test that the data is actually streaming. But it can be useful to test that the end result is correct; for that I like to use |
02ec4c3 to
5516655
Compare
|
Apart from the streaming in the appropriate output format, we might also want take into account the file creation behavior on failure.
|
|
Also not clear how errors mid-stream are handled. Currently this example
results in
which is logical if error mid-stream is considered as break. If this is intended than documentation should specify this semantics and differentiate this from other types of errors. |
|
It's strange that my code results to this error: And then file I think it works as expected..And error message is fine |
rgwood
left a comment
There was a problem hiding this comment.
I think this is a nice improvement, let's land it and try it out. I did some basic testing and the results seemed good.
We need to do some work on general error handling in streams eventually, but I think that's beyond the scope of this work item.
Description
Closes: #7590
User-Facing Changes
So the following command
Will stream data to
output.txtBut I'm note sure how to make a proper test for it, so I leave with no new test cases..
Also rename from
string_binary_list_value_to_bytestovalue_to_bytesto accepts more Value type.Tests + Formatting
Don't forget to add tests that cover your changes.
Make sure you've run and fixed any issues with these commands:
cargo fmt --all -- --checkto check standard code formatting (cargo fmt --allapplies these changes)cargo clippy --workspace -- -D warnings -D clippy::unwrap_used -A clippy::needless_collectto check that you're using the standard code stylecargo test --workspaceto check that all tests passAfter Submitting
If your PR had any user-facing changes, update the documentation after the PR is merged, if necessary. This will help us keep the docs up to date.