Improve command-line tool with options and streaming mode#108
Improve command-line tool with options and streaming mode#108justjake wants to merge 2 commits intojson5:masterfrom
Conversation
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
|
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. |
|
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. |
|
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 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? |
|
We don't need to wait on Browserify for this PR. I would like to see some tests written for this though. |
|
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! |
|
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 |
I tried to pipe something through
json5today to query it withjq, but alas! No support for pipes in the CLI tool.new features:
--stdinto 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.--indentto set pretty-print indent level (default 4)--compileas alias for-cpossibly breaking changes to the tool:
--indentis not 0new dependencies:
cc @reissbaker who introduced me to JSON5 and converted a bunch of files so I can't use
jqon them anymore