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.

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

Improve returned error.message #180

Merged
merged 13 commits into from Mar 6, 2019
Merged

Improve returned error.message #180

merged 13 commits into from Mar 6, 2019

Conversation

ehmicky
Copy link
Collaborator

@ehmicky ehmicky commented Feb 28, 2019

Fix #177

This improves returned error.message by adding information about potential timeouts, signals and exit codes.

This also fixes few issues with result.code:

  • it was an integer instead of a string most of time because of the line error.code = code < 0 ? errname(code) : code. code comes from childProcess.on('exit') which is a positive integer most of the times. The fixed code avoids breaking backward compatibility by using the integer first if it's available.
  • error.errno was not used. There can be instances where process.exit() is not called but error.errno is available, which can be used as error code instead.
  • os.constants.errno is used to retrieve the code string when possible.

This also fixes stderr and stdout being concatenated with a newline between them when options.stdio was not a string.

@sindresorhus
Copy link
Owner

Since technically changing error messages is a breaking change and we have some other potential breaking changes coming up now, I guess we can do a major version after these changes, so we can make breaking changes now.

@sindresorhus
Copy link
Owner

The fixed code avoids breaking backward compatibility by using the integer first if it's available.

We don't have to do this anymore. We just have to make sure we make this clear in the release notes (Note to self).

@sindresorhus
Copy link
Owner

This also fixes few issues with result.code:

Shouldn't these changes we done in a separate PR when fixing #179?

@ehmicky
Copy link
Collaborator Author

ehmicky commented Mar 4, 2019

The changes suggested in #179 can be done in a separate PR IMHO. Note that this PR makes adding those changes much easier.

The backward compatibility fix I mentioned is on error.code. According to #179 it seems like error.code might be removed in next major version anyway, so maybe it's ok to keep this as is in the PR? Or would rather me remove that? To be precise we're talking about this line.

@ehmicky ehmicky changed the title Feature/error message Improve returned error.message Mar 6, 2019
@sindresorhus
Copy link
Owner

According to #179 it seems like error.code might be removed in next major version anyway, so maybe it's ok to keep this as is in the PR?

Sure

@sindresorhus sindresorhus merged commit 1fd9db4 into sindresorhus:master Mar 6, 2019
@ehmicky ehmicky deleted the feature/error_message branch March 6, 2019 17:13
@sindresorhus
Copy link
Owner

This looks good 👌

Would you be interested in joining the project as a maintainer? No worries if not. Just thought I'd ask as you've done some good PRs 😃

@ehmicky
Copy link
Collaborator Author

ehmicky commented Mar 6, 2019

Sure! 👍

My main motivation was that I have two projects closely related to execa:

  • a Portable Node.js guide which relies on execa for cross-platform commands
  • a gulp-execa project not released yet (but almost finished coding). There are about 15 Gulp plugins to trigger shell commands but none of them are cross-platform. All of them use child_process.spawn() as is, except gulp-shell which does some work there but it's quite limited. Also none of them use execa under the hood which I think is the only really maintained Node.js library for this purpose.

@sindresorhus
Copy link
Owner

@ehmicky Awesome. Welcome to the project!

@sindresorhus
Copy link
Owner

Even if you have commit access, it's usually best to do a PR for changes, to get feedback.

@ehmicky
Copy link
Collaborator Author

ehmicky commented Mar 10, 2019

@sindresorhus sounds good. Is it ok to merge other developers PRs if the PR is a straightforward change, or do you prefer always giving it a thumbs up?

@sindresorhus
Copy link
Owner

sounds good. Is it ok to merge other developers PRs if the PR is a straightforward change, or do you prefer always giving it a thumbs up?

I prefer PRs to get multiple reviews. One reviewer might miss something another reviewer might see. I will wait for you to review PRs too as I'm sure you will catch things I might miss too.

@sindresorhus
Copy link
Owner

I could also use your thoughts/comments on the existing issues :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Feature proposal] Improved error messages
2 participants