Skip to content

Commands that have multiple errors now produce cleaner log output#268

Merged
ariporad merged 2 commits intoshelljs:masterfrom
nfischer:MultipleErrorMessages
Jan 14, 2016
Merged

Commands that have multiple errors now produce cleaner log output#268
ariporad merged 2 commits intoshelljs:masterfrom
nfischer:MultipleErrorMessages

Conversation

@nfischer
Copy link
Copy Markdown
Member

@nfischer nfischer commented Dec 29, 2015

This is a fix for #267. The new behavior can be seen to be as such:

$ # assume that fake1, fake2, and fake3 do not exist
$ cat test.js
require('shelljs/global');
ls('fake1', 'fake2', 'fake3');

$ shjs test.js
ls: no such file or directory: fake1
ls: no such file or directory: fake2
ls: no such file or directory: fake3

This is comparable to the Bash behavior:

$ ls fake1 fake2 fake3
ls: cannot access fake1: No such file or directory
ls: cannot access fake2: No such file or directory
ls: cannot access fake3: No such file or directory

Fixes #267, #209.

@nfischer nfischer mentioned this pull request Dec 29, 2015
Closed
@nfischer
Copy link
Copy Markdown
Member Author

nfischer commented Dec 30, 2015

Update: I updated the tests to support the new error output format (which has no trailing newline). I slightly modified the tests by moving the numLines function into a separate utils.js module that is imported (reduces duplication of code).

I added support for stderr. Error messages for commands will now be printed to stderr by default (which is more consistent with unix utilities).

Also, exec() will now respect a process's stderr, and will return a new stderr field (in addition to code and output). This aims to provide a solution to #209. output now refers to only a process's stdout, not all output. This breaks the current behavior, which combines stdout and stderr into output, but it's more consistent with unix, so I think this is the way to go. Both stdout and stderr are outputted to the terminal by default, which is consistent with current behavior.

Because stdout and stderr are no longer combined together into output, this fixes the issue raised in my comment, where shjs commands print everything to stdout.

@nfischer nfischer mentioned this pull request Dec 30, 2015
Closed
@ariporad ariporad added the fix Bug/defect, or a fix for such a problem label Jan 9, 2016
@nfischer nfischer mentioned this pull request Jan 10, 2016
Closed
@nfischer nfischer assigned ariporad and unassigned nfischer Jan 13, 2016
@nfischer
Commands that have multiple errors now produce cleaner log output. Er…
74f1ff8 
…rors are

echoed to stderr. exec() supports a new stderr field.
@nfischer
add exec.stdout
8a7f7ce 
Change `exec.output` to `exec.stdout` and deprecate `output`.
@nfischer
Copy link
Copy Markdown
Member Author

nfischer commented Jan 13, 2016

@ariporad This should be ready for review now. This deprecates the old output property and replaces it with stdout (although output will still contain all stdout). I think using the name stdout is more clear, which is probably the better way to go.

ariporad
ariporad reviewed Jan 13, 2016
View reviewed changes
src/exec.js
Copy link
Copy Markdown
Contributor

@ariporad ariporad Jan 13, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is a duplicate line.

Copy link
Copy Markdown
Member Author

@nfischer nfischer Jan 13, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Which does it duplicate? That's meant to replace the original !fs.existsSync(codeFile) line. It differs from the next line, which uses !fs.existsSync(stdoutFile)

Copy link
Copy Markdown
Contributor

@ariporad ariporad Jan 14, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh yeah, you're right.

@ariporad
Copy link
Copy Markdown
Contributor

ariporad commented Jan 13, 2016

I made one comment, but other than that, LGTM!

@ariporad
Copy link
Copy Markdown
Contributor

ariporad commented Jan 14, 2016

LGTM!

ariporad added a commit that referenced this pull request Jan 14, 2016
@ariporad
Merge pull request #268 from nfischer/MultipleErrorMessages
99f71be 
Commands that have multiple errors now produce cleaner log output
@ariporad ariporad merged commit 99f71be into shelljs:master Jan 14, 2016
@nfischer nfischer deleted the MultipleErrorMessages branch January 16, 2016 04:28
@nfischer nfischer mentioned this pull request Oct 28, 2017
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

@ariporad ariporad

Labels

feature fix Bug/defect, or a fix for such a problem

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Poor output for commands with multiple errors

2 participants

@nfischer @ariporad