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

add to ndjson and to jsonl to the standard library #10519

Merged
merged 9 commits into from
Oct 2, 2023

Conversation

amtoine
Copy link
Member

@amtoine amtoine commented Sep 27, 2023

follow up to

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 😋

User-Facing Changes

users can now convert structured data back to NDJSON and JSONL 👌

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

@amtoine amtoine added std-library Defining and improving the standard library written in nu and the core Rust ccommands new-command labels Sep 27, 2023
Copy link
Member

@sholderbach sholderbach left a comment

Choose a reason for hiding this comment

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

Looks good and thanks for the tests!


# Convert structured data to JSONL.
def "to jsonl" []: any -> string {
each { to json --raw } | to text
Copy link
Member

Choose a reason for hiding this comment

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

Does it make sense to define this in terms of to ndjson? Or do we want to avoid the small performance hit in the std-lib

Copy link
Member Author

Choose a reason for hiding this comment

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

tbh, i just copied the same idea from the from ... commands, i.e. they do not call the other one 😋

@@ -9,12 +9,22 @@
# These functions help `open` the files with unsupported extensions such as ndjson.
#

# Convert from ndjson to structured data.
# Convert from NDJSON to structured data.
Copy link
Member

Choose a reason for hiding this comment

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

Should the help be more explicit?
In theory we could point to the "spec" http://ndjson.org/

Suggested change
# Convert from NDJSON to structured data.
# Convert from newline-delimited JSON to structured data.

Copy link
Member Author

Choose a reason for hiding this comment

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

i went with a259805, is that ok? 😇

@sholderbach
Copy link
Member

sholderbach commented Sep 27, 2023

Suspicious test failures around the to command stub. Apparently the parser grabs that and fails there before looking up the new defintions.

Weird: to and from have basically the exact same signature and are both bound before their respective subcommands.

@sholderbach
Copy link
Member

Marking as draft until the custom to commands work.

@sholderbach sholderbach marked this pull request as draft September 28, 2023 10:47
this commit
- adds space in the expected string output of `to ...`
- adds explicite newlines for the same reason

> **Note**
> we could go with `str trim`, but i wanted to be the output as explicit
> and clear as possible 😌
@amtoine
Copy link
Member Author

amtoine commented Sep 28, 2023

@sholderbach
that was super dumb 😆
now the tests all pass locally 🤞

see 6555677 and af7d4b0, i think they are quite explicite 😆

@amtoine
Copy link
Member Author

amtoine commented Sep 28, 2023

ooh no.... not windows again 😱

@sholderbach
Copy link
Member

OK so a fix to harmonize line ending and we should be good to go?

Does #10429 still persist after the export fix?

@amtoine
Copy link
Member Author

amtoine commented Sep 29, 2023

not really 🤔

after a

use std formats "to ndjson"

the conversion itself is working fine

> ls | first 2 | to ndjson
{"name": "CODE_OF_CONDUCT.md","type": "file","size": 3444,"modified": "2023-04-20 19:41:30.434104143 +02:00"}
{"name": "CONTRIBUTING.md","type": "file","size": 18786,"modified": "2023-09-10 18:34:32.906018569 +02:00"}

but save is confused, as in the issue

> ls | first 2 | save ls.ndjson
Error:   × Internal error: can't run custom command with 'run', use block_id

@amtoine
Copy link
Member Author

amtoine commented Sep 29, 2023

and about the failing test, i think i know what's happening... 😮‍💨

this will prevent having to do a distinction between `\n` on linux
and `\r\n` on windows and should fix the CI.
@amtoine
Copy link
Member Author

amtoine commented Sep 29, 2023

that's such a pain...

@amtoine
Copy link
Member Author

amtoine commented Oct 1, 2023

with 8d5f21c, i see that all standard library tests have passed 🙏

this is then ready for review and CI should go full green very soon 💪

@amtoine amtoine marked this pull request as ready for review October 1, 2023 15:20
Copy link
Member

@sholderbach sholderbach left a comment

Choose a reason for hiding this comment

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

Ship it

@sholderbach sholderbach merged commit 4b9ec03 into nushell:main Oct 2, 2023
19 checks passed
@amtoine amtoine deleted the add-to-ndjson branch October 2, 2023 16:48
hardfau1t pushed a commit to hardfau1t/nushell that referenced this pull request Dec 14, 2023
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
new-command std-library Defining and improving the standard library written in nu and the core Rust ccommands
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants