-
Notifications
You must be signed in to change notification settings - Fork 3
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
introduce support for jsonlines input files #499
base: main
Are you sure you want to change the base?
Conversation
Hi @s4ke, thank you for this. I have a couple of thoughts:
.option('-n, --ndjson', 'JSON Lines Output (also known as Newline Delimited JSON)')
...
.option('-l, --jsonlines', 'JSON Lines Input (also known as Newline Delimited JSON)') What do you think? Does that make it clearer?
How would you feel about making those changes? Or do you have another perspective, in which case I'd be interesting in hearing it? |
Hey @blgm,
|
What do you think about the naming changes? Should we differentiate between input and output in the flags (while still allowing |
Yes, I like this idea because it doesn't break anyone, but provides consistency for any new users I'd be very happy to merge this PR and create a new version if you're able to make the changes discussed above. |
Hi @s4ke. I just wanted to check in on where this PR is. I haven't looked at this yet as the unit tests are failing, so had assumed it was still a work in progress. What are your thoughts? |
Hey @blgm , Sorry, I have not had the time to finish this. Last thing i remembered, was that I had trouble getting the unit test harness to run with my code. We are actually already using the code from our fork in some projects though. No problems so far. Streaming data works like a charm. |
There's no rush @s4ke. |
implements #498
If you are fine with this, I can add the necessary unit tests as well.