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.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

save fails with custom to ndjson command #10429

Closed
amtoine opened this issue Sep 19, 2023 · 5 comments 路 Fixed by #12833
Closed

save fails with custom to ndjson command #10429

amtoine opened this issue Sep 19, 2023 · 5 comments 路 Fixed by #12833
Labels
馃悰 bug Something isn't working investigate this requires investigation scoping/name-resolution How Nu finds which variables/functions are in scope and to what they are bound
Milestone
v0.94.0

Comments

@amtoine
Copy link
Member

amtoine commented Sep 19, 2023

Describe the bug

we now have std formats "from ndjson" but what about to ndjon to be able to hopefully save foo.ndjson? 馃槒

i wanted to try that but i get an error 馃

first of all, here is the command

def "to ndjson" []: any -> string {
    each { to json --raw } | to text
}

How to reproduce

  1. try to dump the output of ls as NDJSON
ls | save foo.ndjson
  1. see the following error
Error:   脳 Internal error: can't run custom command with 'run', use block_id

Expected behavior

i expected this to give the same results as

ls | to ndjson | save foo.ndjson

which gives

> open --raw foo.ndjson
{"name": "CODE_OF_CONDUCT.md","type": "file","size": 3444,"modified": "2023-07-26 13:32:23.457762168 +02:00"}
{"name": "CONTRIBUTING.md","type": "file","size": 18786,"modified": "2023-08-29 12:25:05.674565615 +02:00"}
{"name": "Cargo.lock","type": "file","size": 148614,"modified": "2023-09-19 15:56:31.406434514 +02:00"}
{"name": "Cargo.toml","type": "file","size": 5839,"modified": "2023-09-19 15:56:31.406434514 +02:00"}
{"name": "Cross.toml","type": "file","size": 666,"modified": "2023-08-29 12:25:05.678565620 +02:00"}
{"name": "LICENSE","type": "file","size": 1094,"modified": "2023-07-26 13:32:23.461762176 +02:00"}
{"name": "README.md","type": "file","size": 11990,"modified": "2023-09-11 09:07:31.889059657 +02:00"}
{"name": "assets","type": "dir","size": 4096,"modified": "2023-07-26 13:32:23.473762197 +02:00"}
{"name": "benches","type": "dir","size": 4096,"modified": "2023-09-04 09:06:29.293266800 +02:00"}
{"name": "crates","type": "dir","size": 4096,"modified": "2023-07-26 13:32:23.473762197 +02:00"}
{"name": "docker","type": "dir","size": 4096,"modified": "2023-08-29 12:25:05.778565710 +02:00"}
{"name": "rust-toolchain.toml","type": "file","size": 1105,"modified": "2023-08-29 12:25:05.778565710 +02:00"}
{"name": "scripts","type": "dir","size": 4096,"modified": "2023-09-06 12:38:37.246512586 +02:00"}
{"name": "src","type": "dir","size": 4096,"modified": "2023-09-13 15:05:13.984557075 +02:00"}
{"name": "target","type": "dir","size": 4096,"modified": "2023-07-26 16:45:01.434444741 +02:00"}
{"name": "tests","type": "dir","size": 4096,"modified": "2023-07-26 13:32:23.805762779 +02:00"}
{"name": "toolkit.nu","type": "file","size": 14959,"modified": "2023-09-11 09:07:31.901059560 +02:00"}
{"name": "wix","type": "dir","size": 4096,"modified": "2023-07-26 13:32:23.821762807 +02:00"}

from the Nushell repo 馃憣

Screenshots

No response

Configuration

key value
version 0.84.1
branch main
commit_hash f8939de
build_os linux-x86_64
build_target x86_64-unknown-linux-gnu
rust_version rustc 1.70.0 (90c541806 2023-05-31)
rust_channel 1.70.0-x86_64-unknown-linux-gnu
cargo_version cargo 1.70.0 (ec8a8a0ca 2023-04-25)
build_time 2023-09-19 18:39:26 +02:00
build_rust_channel release
allocator mimalloc
features default, sqlite, trash, which, zip
installed_plugins gstat, nu_plugin_explore

Additional context

No response
@amtoine amtoine added the needs-triage An issue that hasn't had any proper look label Sep 19, 2023
@fdncred
Copy link
Collaborator

fdncred commented Sep 19, 2023

sounds good to me, as long as we can also have a to jsonl that works identically.

@amtoine
Copy link
Member Author

amtoine commented Sep 19, 2023

sounds good to me, as long as we can also have a to jsonl that works identically.

i mean, this is an issue with Nushell's save not working properly with user-defined to ..., right? 馃

@fdncred
Copy link
Collaborator

fdncred commented Sep 19, 2023

Oh, you're saying that you want the implicit conversion by extension if there's a to blah command and the extension is .blah. Hmmm, I'm not sure where that's supposed to happen in save.

@amtoine
Copy link
Member Author

amtoine commented Sep 19, 2023

Oh, you're saying that you want the implicit conversion by extension if there's a to blah command and the extension is .blah.

exactly, just as the symmetric of the behaviour between from blah and save foo.blah 馃憤

Hmmm, I'm not sure where that's supposed to happen in save.

well apart from the only place in the source base where this error is returned, not sure myself either 馃

@amtoine amtoine mentioned this issue Sep 27, 2023
Merged
@sholderbach

This comment was marked as outdated.

Sign in to view
@sholderbach sholderbach added 馃悰 bug Something isn't working scoping/name-resolution How Nu finds which variables/functions are in scope and to what they are bound investigate this requires investigation and removed needs-triage An issue that hasn't had any proper look labels Sep 28, 2023
@sholderbach sholderbach changed the title cannot define to ndjson because of some internal error save fails with custom to ndjson command Sep 29, 2023
sholderbach pushed a commit that referenced this issue Oct 2, 2023
@amtoine
add to ndjson and to jsonl to the standard library (#10519)
4b9ec03 
follow up to
- #10283

# Description
even though it appears defining `to foo` does not allow to do `save
x.foo` for free (see #10429),
because #10283 did add `from ndjson` and `from jsonl` to the standard
library, i thought adding their `to ...` counterpart would make sense
:yum:

# User-Facing Changes
users can now convert structured data back to NDJSON and JSONL :ok_hand:

# Tests + Formatting
this PR adds the exact same tests as for the `from ...` commands
- structured data is in `result` and the string is now the expected
- the two invalid `from ...` tests cannot be reproduced for `to ...`
afaik

# After Submitting
hardfau1t pushed a commit to hardfau1t/nushell that referenced this issue Dec 14, 2023
@amtoine @hardfau1t
add to ndjson and to jsonl to the standard library (nushell#10519)
16986a7 
follow up to
- nushell#10283

# Description
even though it appears defining `to foo` does not allow to do `save
x.foo` for free (see nushell#10429),
because nushell#10283 did add `from ndjson` and `from jsonl` to the standard
library, i thought adding their `to ...` counterpart would make sense
:yum:

# User-Facing Changes
users can now convert structured data back to NDJSON and JSONL :ok_hand:

# Tests + Formatting
this PR adds the exact same tests as for the `from ...` commands
- structured data is in `result` and the string is now the expected
- the two invalid `from ...` tests cannot be reproduced for `to ...`
afaik

# After Submitting
@sholderbach sholderbach mentioned this issue May 2, 2024
Closed
@IanManske IanManske mentioned this issue May 11, 2024
Merged
sholderbach pushed a commit that referenced this issue May 12, 2024
@IanManske
Fix custom converters with save (#12833)
30fc832 
# Description
Fixes #10429 where `save` fails if a custom command is used as the file
format converter.

# Tests + Formatting
Added a test.
@hustcer hustcer added this to the v0.94.0 milestone May 12, 2024
FilipAndersson245 pushed a commit to FilipAndersson245/nushell that referenced this issue May 18, 2024
@IanManske @FilipAndersson245
Fix custom converters with save (nushell#12833)
332513a 
# Description
Fixes nushell#10429 where `save` fails if a custom command is used as the file
format converter.

# Tests + Formatting
Added a test.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
馃悰 bug Something isn't working investigate this requires investigation scoping/name-resolution How Nu finds which variables/functions are in scope and to what they are bound
Projects
None yet
Milestone
v0.94.0
Development

Successfully merging a pull request may close this issue.

Fix custom converters with save IanManske/nushell
4 participants
@fdncred @hustcer @sholderbach @amtoine