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

Adds support for `Boom.someMethod(err)`, closes #138 #139

Merged
merged 3 commits into from Mar 21, 2017

Conversation

@niftylettuce
Copy link
Contributor

niftylettuce commented Oct 26, 2016

No description provided.

niftylettuce added a commit to ladjs/koa-better-error-handler that referenced this pull request Oct 26, 2016
@arb

This comment has been minimized.

Copy link
Contributor

arb commented Oct 27, 2016

Why can't you just call wrap directly?

@niftylettuce

This comment has been minimized.

Copy link
Contributor Author

niftylettuce commented Oct 27, 2016

@arb as I mentioned, I do not call wrap directly because it's more convenient and readable to be able to use methods like Boom.badRequest(err) instead of Boom.wrap.

@arb

This comment has been minimized.

Copy link
Contributor

arb commented Nov 21, 2016

I don't think it's more readable. I think it makes the API a little more difficult to read. wrap is to wrap an existing error.

@niftylettuce

This comment has been minimized.

Copy link
Contributor Author

niftylettuce commented Nov 21, 2016

Oh come on it's another helper method! I submitted PR it passes all tests
(I even added some). Do I really need to fork and publish boom of my own
to NPM for something this simple?.... Readable is opinionated. This just
is a helper method.

On Nov 21, 2016 3:37 PM, "Adam Bretz" notifications@github.com wrote:

I don't think it's more readable. I think it makes the API a little more
difficult to read. wrap is to wrap an existing error.


You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
#139 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AAf7hVqV2pnh4WBvlnBaLu0jjsRtvL1Kks5rAgEWgaJpZM4KhptQ
.

@AdriVanHoudt

This comment has been minimized.

Copy link
Contributor

AdriVanHoudt commented Nov 22, 2016

I could personally see the value of this. Boom is handy so I don't need to remember all the status codes. Making the wrapping easier seems fine to me.

@arb

This comment has been minimized.

Copy link
Contributor

arb commented Nov 22, 2016

The threat of forking really does not help your case. If that is what you want to do, I encourage you to do so.

It's not "adding" anything, it's changing the meaning of create.

I am against this, but if the community supports it, I'll concede.

@hueniverse

This comment has been minimized.

Copy link
Member

hueniverse commented Nov 24, 2016

I don't mind this change. I think the error should be wrapped first, then set data, not the way it is now.

@niftylettuce you should never use forking as an argument for getting a PR merged. Regardless of what was intended, it comes off as an attack on the lead maintainer and it never a productive move.

expect(err.name).to.equal('ValidationError');
expect(err.message).to.equal('An example mongoose validation error');
done();

This comment has been minimized.

Copy link
@arb

arb Nov 29, 2016

Contributor

Extra whitespace.

const err = Boom[name](error, { foo: 'bar' });
expect(err.data).to.equal({ foo: 'bar' });
done();

This comment has been minimized.

Copy link
@arb

arb Nov 29, 2016

Contributor

Extra whitespace.

@arb

This comment has been minimized.

Copy link
Contributor

arb commented Dec 20, 2016

@niftylettuce are you still interested in getting this in? Please see my and hueniverse comments. If not, I plan on closing this PR.

@niftylettuce

This comment has been minimized.

Copy link
Contributor Author

niftylettuce commented Feb 6, 2017

pushed update without white space

@arb

This comment has been minimized.

Copy link
Contributor

arb commented Feb 6, 2017

Please see @hueniverse comment about wrapping first, then setting data.

@travi

This comment has been minimized.

Copy link

travi commented Feb 27, 2017

i would see this change as valuable as well. anything i can do to contribute to getting this pushed through?

@hueniverse hueniverse dismissed arb’s stale review Mar 21, 2017

Will fix.

@hueniverse hueniverse self-assigned this Mar 21, 2017
@hueniverse hueniverse added the feature label Mar 21, 2017
@hueniverse hueniverse added this to the 4.3.0 milestone Mar 21, 2017
@hueniverse hueniverse merged commit c803162 into hapijs:master Mar 21, 2017
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
hueniverse added a commit that referenced this pull request Mar 21, 2017
@travi

This comment has been minimized.

Copy link

travi commented Mar 21, 2017

thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.