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

ifError should throw its argument #369

Closed
Magomogo opened this issue Feb 17, 2015 · 9 comments
Closed

ifError should throw its argument #369

Magomogo opened this issue Feb 17, 2015 · 9 comments

Comments

@Magomogo
Copy link

Why ifError(arg) doesn't throw its argument as node.js' included assert module do?

@keithamus
Copy link
Member

Hi @Magomogo thanks for the issue.

As far as I know, Assert.ifError works exactly the same way as Node.js' Assert.ifError: It throws on truthy values.

assert.ifError(false);
assert.ifError(null);
assert.ifError(undefined);

err(function () {
  assert.ifError('foo');
 }, "expected \'foo\' to be falsy");

If in doubt though, read the source. Here is Node.js' source for ifError, while here is chai's. Chai's is a little more complex, so it can interface with the standard assertion errors. Here's the ok assertion which as you can see just passes in the object as the assertion, and finally here is the underlying coercion logic. Ultimately it all resolves to be the exact behaviour that Node's assert.isError exhibits.

Do you have a specific code example that isn't working?

@Magomogo
Copy link
Author

Hi, @keithamus!

Thanks for answering.
Just take a look on this sample:

var assert = require('chai').assert,
    error = new Error('Ouch!');

assert.ifError(error);

It gives AssertionError: expected [Error: Ouch!] to be falsy instead of an exception with stack trace.

@keithamus
Copy link
Member

Ah. I should perhaps take some of my own advice and read the source! It does indeed behave differently. It seems like our version of ifError will throw an AssertionError if the given argument is not falsey, while node will throw the given argument.

I can't really tell you why this is different - but I can say that it is unlikely going to change any time soon; we don't have metrics on how many users use this feature, but cannot reasonably change the behaviour without potentially breaking existing users' code.

@Magomogo, if you're looking for the same behaviour as Node's assert.ifError - it should be trivial to simply inline it (if (error) throw error;). As I say, it is unfortunate that the two methods share the same name but not the same behaviour, but we're kind of stuck with our implementation for the sake of compatibility.

@Magomogo
Copy link
Author

I understand your concerns. However it's bad that one can't just replace standard assert library with yours. Maybe makes sense to fix this behavior in major versions.
Thanks and good luck!

@lexi-sh
Copy link

lexi-sh commented Mar 14, 2015

Hey @keithamus , should there be a new 3.0 branch that I pull request to if I make this change?

@keithamus
Copy link
Member

@cjqed I've just created a 3.x.x branch for you - if you make a PR to that, that will be great 😄

@lexi-sh
Copy link

lexi-sh commented Mar 16, 2015

So, I tried to look up things about ifError, like how it came to be and the documentation surrounding it, but:

  1. The comments in the code don't say anything:
  /*!
   * Undocumented / untested
   */

  assert.ifError = function (val) {
  1. And then the only thing I can find is this PR: New assertion: isError #145

I'm not really sure what the expected behavior in 3.0 should be. Should we just mimic node behavior?

  assert.ifError = function (val) {
    if (val) {
      throw(val);
    }
  };

@keithamus
Copy link
Member

Absolutely, just mimic the Node API 100%. If it helps, tests for Node's assert.ifError can be found here and here

@toymachiner62
Copy link

What is the expect equiavlent of assert.ifError?

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

No branches or pull requests

5 participants