Error message can be better. Might be something like

• Version should be one of patch, minor, major or a valid semver version

Version should be either path, minor, major, or a valid semver version

Somehow we should inform the user that patch is the default.

Yup. Don't know if there's any syntax convention for default values though. Might just write it in text below.

$ np [patch | minor | major | <version>] (Default: patch)

or

$ np [patch | minor | major | <version>]
patch is default

Former

Why?

I thought that if stderr contains data (for example when running npm install) we should throw to inform the user.

Nah, stderr doesn't mean error. It's just an alternative stream commonly used for errors. The exit code decides whether something is an error.

Instead just log both stdout and stderr in this method. The return is also moot as it's not used anywhere.

It's used in the if statements.

if (exec('git', ['status', '--porcelain']) !== '') {

So just do something like this and print all the output?

const exec = (cmd, args) => {
    const {stdout, stderr} = execa.sync(cmd, args);
    console.log(stdout);
    console.log(stderr);        

    return stdout;
};

Going to drop using it in the if statements. It doesn't need logging for git status --porcelain.

👍

stderr should be logged with console.error though.

Should I only log if stdout or stderr is not empty? To prevent from printing empty lines.

Actually, might be better to just pass stdio: 'inherit' in the options, instead of console.

It doesn't look like stdio is an option in execa

It's probably passed somewhere down the chain. If I pass in stdio: 'inherit' I get the error Cannot read property 'length' of null when running execa.sync('npm', ['install'], {stdio: 'inherit'});

Alright. Just use console.log/err then.

Do you get the error even with the latest master of execa? If so, would you mind opening an issue? stdio should definitely be supported.

It seems master works.

I think it is supported in master since we use childProcess.spawn there. In 0.4.0 we're still using execFile which doesn't have a stdio option.

Is it possible to do a new execa release or should I use console.log/err for now and create a new issue here to keep track of this improvement.

Just use console.log/err for now, but add a TODO comment about switching when next execa version is out. I don't think we're ready to do a release there just yet.

Left-over coverage.

Does this actually work? I updated sindre-playground via a separate clone and ran the command

$ git rev-list --count --left-only @'{u}'...HEAD
0

Is it possible that we should run git fetch first?

Tested it with running git fetch first and then the rev-list check works.

Should we call process.exit(1) here? If np is used in another CLI tool it should stop executing if something went wrong, no?

Yes, and should also use console.error.