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
Add json output to ls
and updated
commands
#824
Conversation
Node 4 test failure(s) appear to be unrelated... 👀 |
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.
Looks good so far, thanks @ricky!
test/UpdatedCommand.js
Outdated
if (err) return done.fail(err); | ||
try { | ||
const outputStr = consoleOutput(); | ||
expect(outputStr).toMatchSnapshot(); |
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.
I think we can just skip straight to the JSON.parse(consoleOutput())
snapshot, as we really shouldn't be testing whether or not JSON.stringify()
is working correctly.
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.
Fixed in 73f492a
src/Command.js
Outdated
@@ -70,6 +78,11 @@ export default class Command { | |||
log.pause(); | |||
log.heading = "lerna"; | |||
|
|||
// Default to silent log level for JSON output | |||
if (flags.json) { |
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.
This isn't necessary, actually, as all logging is emitted to stderr
and only the JSON string is emitted to stdout
. When you pipe the output, it only includes stdout
(unless you do fancy stdio redirects, of course).
It's too bad I didn't write a lerna ls
integration test that could demonstrate this :/
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.
Removed in ceb1fe0
src/Command.js
Outdated
(Only for 'ls' and 'updated' commands) | ||
`, | ||
type: "boolean", | ||
default: false |
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.
This default is implicit for boolean options.
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.
src/Command.js
Outdated
@@ -62,6 +62,14 @@ export const builder = { | |||
describe: "Set max-buffer(bytes) for Command execution", | |||
type: "number", | |||
requiresArg: true | |||
}, | |||
"json": { | |||
describe: dedent` |
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.
Let's move the json
option descriptor into LsCommand
and UpdatedCommand
directly. It's an acceptable amount of duplication to maintain the benefit of targeted, relevant --help
output. The description could be shorter, like "Show information in JSON format". (when in doubt, i copy npm's docs)
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.
Fixed in ceb1fe0
And yes, the node v4 failure was due to a timeout. That's unfortunate, and hopefully rare? |
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 the feedback!
src/Command.js
Outdated
@@ -62,6 +62,14 @@ export const builder = { | |||
describe: "Set max-buffer(bytes) for Command execution", | |||
type: "number", | |||
requiresArg: true | |||
}, | |||
"json": { | |||
describe: dedent` |
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.
Fixed in ceb1fe0
src/Command.js
Outdated
(Only for 'ls' and 'updated' commands) | ||
`, | ||
type: "boolean", | ||
default: false |
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.
src/Command.js
Outdated
@@ -70,6 +78,11 @@ export default class Command { | |||
log.pause(); | |||
log.heading = "lerna"; | |||
|
|||
// Default to silent log level for JSON output | |||
if (flags.json) { |
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.
Removed in ceb1fe0
test/UpdatedCommand.js
Outdated
if (err) return done.fail(err); | ||
try { | ||
const outputStr = consoleOutput(); | ||
expect(outputStr).toMatchSnapshot(); |
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.
Fixed in 73f492a
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.
Looks great, thanks!
This thread has been automatically locked because there has not been any recent activity after it was closed. Please open a new issue for related bugs. |
Description
Adds a
--json
flag, which is currently used only bylerna ls
andlerna updated
to produce machine-readable output.Motivation and Context
Inspired by the discussion in #686 and my own desire to not have our artisanally-crafted script be reliant on the plain text formatting of
lerna updated
moving forward.How Has This Been Tested?
Manually via invocations of the command with and without the
--json
flag, as well as new test cases.Types of changes
Checklist: