[go: up one dir, main page]

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

introduce support for jsonlines input files #499

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

s4ke
Copy link
@s4ke s4ke commented Feb 20, 2021

implements #498

If you are fine with this, I can add the necessary unit tests as well.

@blgm
Copy link
Owner
blgm commented Feb 23, 2021

Hi @s4ke, thank you for this. I have a couple of thoughts:

  1. As far as I can see "jsonlines" and "ndjson" are essentially the same thing. I don't have an opinion on which name is best, but I think it makes jfq easier to understand if we make it clear that they are the same (if that is indeed correct), and I think we could make that clear in the options descriptions like this:
.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?

  1. I'd like to see at least one test for this, probably in the file src/__tests__/jfq_inputs.js. I know that people have different opinions on testing. The reason I'd like one is that I don't run any manual tests before creating a new version of jfq, and I don't want it to be accidentally broken by a future change to the code or to one of the dependencies.

How would you feel about making those changes? Or do you have another perspective, in which case I'd be interesting in hearing it?

@s4ke
Copy link
Author
s4ke commented Feb 23, 2021

Hey @blgm,

  1. Yes, they are indeed the same, but since I did not want to break ndjson in an backwards incompatible way by enforcing the input to be new line delimited json as well I opted for the separate flag. Tbh, something like --input-ndjson and --output-ndjson (or the counterpart with jsonlines in the name) should probably be better?

  2. sure. I just wanted to check in whether this is something you would be willing to add to this lib. And yes, I totally agree with you, this needs more tests!

@s4ke
Copy link
Author
s4ke commented Feb 25, 2021

What do you think about the naming changes? Should we differentiate between input and output in the flags (while still allowing ndjson as an alias to --output-jsonlines)?

@blgm
Copy link
Owner
blgm commented Feb 25, 2021

Should we differentiate between input and output in the flags (while still allowing ndjson as an alias to --output-jsonlines)?

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.

@blgm
Copy link
Owner
blgm commented May 1, 2021

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?

@s4ke
Copy link
Author
s4ke commented May 1, 2021

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.

@blgm
Copy link
Owner
blgm commented May 3, 2021

There's no rush @s4ke.

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