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 command-line tool with options and streaming mode #108

Closed
wants to merge 2 commits into from

Conversation

justjake
Copy link

I tried to pipe something through json5 today to query it with jq, but alas! No support for pipes in the CLI tool.

new features:

  • add option --stdin to stream JSON5 in, and JSON out. This is implemented in the most naive way imaginable (trying to parse one character at a time!), but it works, and it's easily fast enough for my command-line scripting needs.
  • add option --indent to set pretty-print indent level (default 4)
  • add option --compile as alias for -c

possibly breaking changes to the tool:

  • always write a trailing newline at the end of JSON output if --indent is not 0
  • probably requires node >= 0.10 for streams support

new dependencies:

  • adds "yargs" as a dependency for command-line option parsing

cc @reissbaker who introduced me to JSON5 and converted a bunch of files so I can't use jq on them anymore

Jake Teton-Landis added 2 commits February 9, 2016 20:49
new features:
- add option `--stdin` to stream JSON5 in, and JSON out
- add option `--indent` to set pretty-print indent level (default 4)
- add option `--compile` as alias for `-c`

possibly breaking changes:
- write trailing newlines if `--indent` is not 0

new dependencies:
- adds "yargs" as a dependency for command-line option parsing
@aseemk
Copy link
Member

aseemk commented Feb 10, 2016

Nice! Thanks for the great contribution.

I'll try to review this when I get some time, but hopefully one of the other maintainers will beat me.

@jordanbtucker
Copy link
Member

I'm all for adding this functionality, and the code looks good at a glance. I'll look over it more closely later, but for now I'd just like to point out that this would set a precedent for adding NPM dependencies.

Historically, we've avoided those to make it easier for web developers to use JSON5. Granted, this PR only changes the CLI and not the core, so we could merge this now, but we should probably implement Browserify before we start adding more dependencies. I took a crack at it but ultimately had to revert it. If someone would like to pick up where I left off, that would be awesome.

@justjake
Copy link
Author

I don't think adding an NPM dependency to the command-line tool significantly changes the requirements to use the tool. This won't break anyone's scripts

a) the command-line tool already uses require() to load the JSON5 module as a dependency, so it's fine to introduce an additional use of require().
b) you already recommend installing the tool as an NPM package, so adding an NPM dependency doesn't significantly change the interface; node comes with npm on all platforms but FreeBSD.

I can understand not wanting to move forward with additional dependencies without Browserify -- future contributors might want to use NPM deps in JSON5.js itself, and then where would we be? If that's the case, would I be better off publishing my own json5-tool package or something?

@jordanbtucker
Copy link
Member

We don't need to wait on Browserify for this PR. I would like to see some tests written for this though.

@aseemk
Copy link
Member

aseemk commented Feb 21, 2016

Big +1 to an npm dependency being okay here.

I also agree some tests would be nice, so no one has to manually test whenever making changes to the CLI code, but they can be basic tests for the logic rather than e.g. the command-line argument parsing (since you're using a dependency for that).

Thanks again!

@jordanbtucker
Copy link
Member

jordanbtucker commented Sep 21, 2017

Partially fixed in 35269da.

It currently does not stream JSON5 input, but consumes it all at once, however the parser is capable of parsing streaming input with some API changes. It still uses JSON.stringify, so output is not streamed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants