-
-
Notifications
You must be signed in to change notification settings - Fork 287
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
Improve JSONL handling #1730
Conversation
e695566
to
8598566
Compare
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:
|
We can sit on it for a bit while you're actively developing, see if we want to tweak the message or anything. |
623a0bb
to
f54e54d
Compare
@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:
From a user perspective, there is no change if they save as I haven't added any tests yet, so I'll look into that. |
f54e54d
to
7f9d324
Compare
EDIT: Sorry, I think I understand now...
|
Hmm... Testing has revealed a bug when saving as |
I think we're good now. Edit: Helps to commit the sample data... Edit 2: Removed some vestigial code and force pushed |
9877398
to
cbb92f2
Compare
cbb92f2
to
c9c3142
Compare
c9c3142
to
703914c
Compare
There was a problem hiding this 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.
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"} |
There was a problem hiding this comment.
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.
Work in progress for #1726