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.

Sign up for GitHub

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

Don't fail on stderr output, but print the output to stderr #110

Merged
merged 4 commits into from
Sep 25, 2016
Merged

Don't fail on stderr output, but print the output to stderr #110

Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
8f0623b
test: add test for not exiting on stderr content and match stderr output
Tapppi Sep 22, 2016
82cabab
feat: don't fail fast on stderr content, and print stderr, closes #91
Tapppi Sep 22, 2016
458da35
fix: print warnings in yellow instead of red
Sep 23, 2016
6338f32
style: simplify handleExecError if logic
Tapppi Sep 24, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 3 additions & 2 deletions index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -109,10 +109,11 @@ function outputChangelog (argv, cb) {
}

function handleExecError (err, stderr) {
Copy link
Member

Choose a reason for hiding this comment

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

I thought we should handle err and stderr differently?
the problem is with stderr shouldn't terminate the program. But for err we keep the same behaviour. No?

Copy link
Member

Choose a reason for hiding this comment

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

I think I misread your code. It LGTM now.

// If exec returns an error or content in stderr, log it and exit with return code 1
// If exec returns content in stderr, but no error, print it as a warning
// If exec returns an error, print it and exit with return code 1
var errMessage = stderr || (err && err.message)
if (errMessage) {
console.error(chalk.red(errMessage))
console.error(err ? chalk.red(errMessage) : chalk.yellow(errMessage))
if (err) {
process.exit(1)
}
Copy link
Contributor

Choose a reason for hiding this comment

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

@Tapppi , i think it would be better to be:

    if (err) {
      console.error(chalk.red(stderr || err.message))
      process.exit(1)
    } else if(stderr) {
      console.warn(chalk.yellow(stderr))
    }

the prior stderr in console.error could be dropped.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good idea, changed in 6338f32

Expand Down