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

Show file tree
Hide file tree
Changes from 2 commits
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
31 changes: 14 additions & 17 deletions index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -108,6 +108,17 @@ 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
var errMessage = stderr || (err && err.message)
if (errMessage) {
console.error(chalk.red(errMessage))
Copy link
Contributor

Choose a reason for hiding this comment

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

i think when err is null, better to use console.warn to output the error message. Red message means real error.
Since console.error/warn's output will have its own color, should it continue to use chalk to control the color?

Copy link
Member

Choose a reason for hiding this comment

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

Since console.error/warn's output will have its own color

I don't think so

Copy link
Member Author

Choose a reason for hiding this comment

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

The console.warn() function is an alias for console.error().
https://nodejs.org/api/console.html#console_console_warn_data

Copy link
Member Author

@Tapppi Tapppi Sep 23, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

I think we should print to stderr (console.error), but how about yellow color for when the exit code was 0?

Since console.error/warn's output will have its own color, should it continue to use chalk to control the color?

AFAIK chalk should handle coloring the output correctly regardless of the stream it will be printed to.

Copy link
Contributor

Choose a reason for hiding this comment

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

sorry for my poor knowledge, yellow would be great!

Copy link
Member Author

Choose a reason for hiding this comment

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

Done in 458da35

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

}
}

function commit (argv, newVersion, cb) {
var msg = 'committing %s'
var args = [argv.infile]
Expand All @@ -118,14 +129,6 @@ function commit (argv, newVersion, cb) {
}
checkpoint(msg, args)

function handleExecError (err, stderr) {
// If exec returns an error or content in stderr, log it and exit with return code 1
var errMessage = stderr || (err && err.message)
if (errMessage) {
console.log(chalk.red(errMessage))
process.exit(1)
}
}
exec('git add package.json ' + argv.infile, function (err, stdout, stderr) {
handleExecError(err, stderr)
exec('git commit ' + verify + (argv.sign ? '-S ' : '') + 'package.json ' + argv.infile + ' -m "' + formatCommitMessage(argv.message, newVersion) + '"', function (err, stdout, stderr) {
Expand All @@ -148,17 +151,11 @@ function tag (newVersion, argv) {
}
checkpoint('tagging release %s', [newVersion])
exec('git tag ' + tagOption + 'v' + newVersion + ' -m "' + formatCommitMessage(argv.message, newVersion) + '"', function (err, stdout, stderr) {
handleExecError(err, stderr)
var message = 'git push --follow-tags origin master'
var errMessage = null
if (err) errMessage = err.message
if (stderr) errMessage = stderr
if (pkg.private !== true) message += '; npm publish'
if (errMessage) {
console.log(chalk.red(errMessage))
process.exit(1)
} else {
checkpoint('Run `%s` to publish', [message], chalk.blue(figures.info))
}

checkpoint('Run `%s` to publish', [message], chalk.blue(figures.info))
})
}

Expand Down
20 changes: 17 additions & 3 deletions test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -117,7 +117,7 @@ describe('cli', function () {

var result = execCli()
result.code.should.equal(1)
result.stdout.should.match(/commit yourself/)
result.stderr.should.match(/commit yourself/)

unmock()
})
Expand All @@ -131,7 +131,7 @@ describe('cli', function () {

var result = execCli()
result.code.should.equal(1)
result.stdout.should.match(/addition is hard/)
result.stderr.should.match(/addition is hard/)

unmock()
})
Expand All @@ -145,7 +145,21 @@ describe('cli', function () {

var result = execCli()
result.code.should.equal(1)
result.stdout.should.match(/tag, you're it/)
result.stderr.should.match(/tag, you're it/)

unmock()
})
})

it('doesn\'t fail fast on stderr output from git', function () {
// mock git by throwing on attempt to commit
return mockGit('console.error("haha, kidding, this is just a warning"); process.exit(0);', 'add')
.then(function (unmock) {
writePackageJson('1.0.0')

var result = execCli()
result.code.should.equal(0)
result.stderr.should.match(/haha, kidding, this is just a warning/)

unmock()
})
Expand Down