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

better error handling #36

Merged
merged 3 commits into from
Dec 8, 2015
Merged

better error handling #36

merged 3 commits into from
Dec 8, 2015

Conversation

timaschew
Copy link
Contributor

  • provide error handling for callback
  • provide default error handling if no callback is passed

- provide error handling for callback
- provide default error handling if no callback is passed
try {
(callback || function(err) {
if (!err) {
console.log(err)
Copy link
Owner

Choose a reason for hiding this comment

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

Do you really mean to only log here if !err?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. lol no I mean err != null but this was criticized by the linter, of course I mean !!err

I think if the caller is not interested in handling any successful callback, because it's the last action anyway or so, then he should be notified in the error case.

But you can also omit the logging in the binary here https://github.com/tschaub/gh-pages/blob/master/bin/gh-pages#L35
and do a successful log in this file.

Copy link
Owner

Choose a reason for hiding this comment

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

I'm just confused why you would want to log undefined or other falsy values.

To be clear, did you mean to write this:

if (err) {
  console.log(err);
}

instead of this:

if (!err) {
  console.log(err);
}

?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, you're right, I edit my previous answer.
But if (err){} didn't work, the error is falsy, so !!err handles it the right way or err != null

Copy link
Contributor Author

Choose a reason for hiding this comment

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

and err != null was linted with this error: gh-pages/lib/index.js 63:13 error Use ‘===’ to compare with ‘null’ no-eq-null

which is really sad, because null check with != makes sense to check against undefined and null

Copy link
Contributor Author

Choose a reason for hiding this comment

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

haha !! is not allowed too.

So either one of the rules needs to be disabled or the check should be
if (typeof err !== 'undefined' && err !== null) {}

Copy link
Owner

Choose a reason for hiding this comment

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

@timaschew you can just write this:

if (err) {
  // do something
}

This behaves exactly the same as this:

if (!!err) {
  // do something
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's true ^^
I tried this in the morning today, but seems that I was confused a little bit...

@timaschew timaschew changed the title better error handling, fix #20 better error handling Dec 7, 2015
tschaub added a commit that referenced this pull request Dec 8, 2015
@tschaub tschaub merged commit 0794c0b into tschaub:master Dec 8, 2015
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.

None yet

2 participants