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

Improve JSONL handling #1730

Merged
merged 9 commits into from
Feb 15, 2023
Merged

Improve JSONL handling #1730

merged 9 commits into from
Feb 15, 2023

Conversation

daviewales
Copy link
Contributor

Work in progress for #1726

@daviewales daviewales marked this pull request as draft February 9, 2023 09:43
@daviewales daviewales changed the title WIP: Error when saving jsonl file with no rows #1726 Error when saving jsonl file with no rows #1726 Feb 9, 2023
@daviewales daviewales changed the title Error when saving jsonl file with no rows #1726 Improve JSONL handling Feb 9, 2023
@daviewales
Copy link
Contributor Author

daviewales commented Feb 11, 2023

I was thinking of bundling the following into this PR, but also happy if you want to merge this and I'll send another PR when the rest is ready:

  • Add warning when saving jsonl file with no rows
  • Add JSONL arrays loader
  • Add JSONL arrays saver
  • Add option to save JSONL as arrays
  • Add extended warning which hints to use JSONL arrays format to preserve headers
  • Add tests

@saulpw
Copy link
Owner

saulpw commented Feb 11, 2023

We can sit on it for a bit while you're actively developing, see if we want to tweak the message or anything.

@daviewales daviewales force-pushed the jsonl-tweaks branch 2 times, most recently from 623a0bb to f54e54d Compare February 14, 2023 10:01
@daviewales
Copy link
Contributor Author

daviewales commented Feb 14, 2023

@saulpw, I think I'm ready for you to look at this now. I've made a few potentially controversial choices, so keen to hear your thoughts! A summary of my questionable choices follows:

  • JSONL arrays get their own sheet type, inheriting from SequencSheet
  • JSONL arrays get their own file extension (.jsonla)
  • Loading as JSONL arrays automatically falls back to the default JSONL loader if the file doesn't include a JSONL arrays header
  • I've split the json_save_arrays option in two: json_save_arrays and json_load_arrays. The first will always use the arrays format for .jsonl files, and the second will always attempt use the arrays format for .jsonl files, falling back to the default if the header is not detected.

From a user perspective, there is no change if they save as .jsonl and have the new options unset.

I haven't added any tests yet, so I'll look into that.

@daviewales
Copy link
Contributor Author

daviewales commented Feb 14, 2023

EDIT: Sorry, I think I understand now...

Perhaps I don't understand how testing works.
But it looks to me that the existing save-json.vd test doesn't actually save anything?
https://github.com/saulpw/visidata/blob/develop/tests/save-json.vd

@daviewales
Copy link
Contributor Author

Hmm... Testing has revealed a bug when saving as .jsonla.

@daviewales
Copy link
Contributor Author

daviewales commented Feb 14, 2023

I think we're good now.

Edit: Helps to commit the sample data...

Edit 2: Removed some vestigial code and force pushed

@daviewales daviewales marked this pull request as ready for review February 14, 2023 12:07
Copy link
Owner

@saulpw saulpw left a comment

Choose a reason for hiding this comment

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

Thanks for pulling this together, @daviewales ! I had a few comments, and if we address those I think we should be able to merge it.

tests/load-jsonla.vdj Outdated Show resolved Hide resolved
tests/save-jsonla.vdj Show resolved Hide resolved
visidata/loaders/json.py Outdated Show resolved Hide resolved
visidata/loaders/json.py Outdated Show resolved Hide resolved
visidata/loaders/json.py Outdated Show resolved Hide resolved
@daviewales
Copy link
Contributor Author

Thanks @saulpw, I've pushed some commits with the requested changes.

@@ -0,0 +1,3 @@
#!vd -p
{"longname": "open-file", "input": "sample_data/officials.jsonla", "keystrokes": "o"}
{"sheet": "officials", "col": "", "row": "", "longname": "columns-sheet", "input": "", "keystrokes": "Shift+C", "comment": "open Columns Sheet: edit column properties for current sheet"}
Copy link
Owner

Choose a reason for hiding this comment

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

ah, here I meant also to not open the columns sheet :) I'll fix this after merging.

@saulpw saulpw merged commit 1dad94d into saulpw:develop Feb 15, 2023
@daviewales daviewales deleted the jsonl-tweaks branch March 15, 2023 21:37
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.

2 participants