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

Add Boom.internal() to API docs fixes #127 #129

Merged
merged 1 commit into from Sep 27, 2016

Conversation

@tribou
Copy link
Contributor

tribou commented Sep 24, 2016

I followed the Joi API doc example and added internal as an alias for badImplementation since the returned statusCode and error are the same.

@arb

This comment has been minimized.

Copy link
Contributor

arb commented Sep 26, 2016

I think I'm going to pass on this. I don't think I've ever once seen this used in the wild. Plus it makes the documentation for this look really ugly.

@arb arb closed this Sep 26, 2016
@Marsup

This comment has been minimized.

Copy link
Member

Marsup commented Sep 26, 2016

@arb you authorized it in #127 :D

@arb

This comment has been minimized.

Copy link
Contributor

arb commented Sep 26, 2016

That was the old me.

@mtharrison

This comment has been minimized.

Copy link
Member

mtharrison commented Sep 26, 2016

@tribou

This comment has been minimized.

Copy link
Contributor Author

tribou commented Sep 26, 2016

Well, thanks for at least considering, and I respect the fact that this hapi ecosystem is run as a tight ship (which is why I use it). 😉

FYI, I believe I got it from @geek who used it in his Nodevember 2015 presentation on Seneca:

https://github.com/geek/nodevember-2015/blob/6d7793cfe60c557945042f026e165532740a3f4f/arrival_open_routeV2.js

So he might have used it other places as well? My main concern for #127 is that Boom.internal() doesn't get accidentally removed without a major version bump, since I was planning to use it in a production environment (depending on the outcome of this issue). 😄

...it also looks like it's in one of the Hapi tests, but I don't know if that's enough of a reason to document it:

https://github.com/hapijs/hapi/blob/ed5e75e467e7c5744f6dc58cd067ae1d8bab18a5/test/handler.js#L378

@AdriVanHoudt

This comment has been minimized.

@Marsup

This comment has been minimized.

Copy link
Member

Marsup commented Sep 26, 2016

@tribou if it's already an alias to badImplementation, why not use this one then ?

@tribou

This comment has been minimized.

Copy link
Contributor Author

tribou commented Sep 26, 2016

@Marsup yep, you're right. I would just need to go back and change all of the places I had used it before realizing it wasn't in the API docs...

But is it still an issue that internal() may be currently used in prod environments and needs to be preserved? (Though I'm still confused as to where it all originated.)

@arb

This comment has been minimized.

Copy link
Contributor

arb commented Sep 26, 2016

I guess my question is why are you using it over badImplementation? It is a public method so it should be documented... just not sure I understand the use case.

@geek

This comment has been minimized.

Copy link
Member

geek commented Sep 26, 2016

@arb 500 is an internal server error, seems obvious to use the method named internal

@arb arb reopened this Sep 26, 2016
README.md Outdated
@@ -593,7 +593,7 @@ Generates the following response payload:

All 500 errors hide your message from the end user. Your message is recorded in the server log.

### `Boom.badImplementation([message], [data])`
### `Boom.badImplementation([message], [data])` - alias: `internal`

This comment has been minimized.

Copy link
@arb

arb Sep 26, 2016

Contributor

Can you change this to
Boom.badImplementation([message], [data]) - (alias: internal)

README.md Outdated
@@ -37,7 +37,7 @@ Lead Maintainer: [Adam Bretz](https://github.com/arb)
- [`Boom.tooManyRequests([message], [data])`](#boomtoomanyrequestsmessage-data)
- [`Boom.illegal([message], [data])`](#boomillegalmessage-data)
- [HTTP 5xx Errors](#http-5xx-errors)
- [`Boom.badImplementation([message], [data])`](#boombadimplementationmessage-data)
- [`Boom.badImplementation([message], [data])` - alias: `internal`](#boombadimplementationmessage-data---alias-internal)

This comment has been minimized.

Copy link
@arb

arb Sep 26, 2016

Contributor

Undo this please. The TOC is automatically generated.

@tribou tribou force-pushed the tribou:127-boom-internal-docs branch from 8a65bfb to 1cd79ad Sep 27, 2016
@tribou

This comment has been minimized.

Copy link
Contributor Author

tribou commented Sep 27, 2016

Both changes should now be updated.

@arb
arb approved these changes Sep 27, 2016
@arb arb self-assigned this Sep 27, 2016
@arb arb added the documentation label Sep 27, 2016
@arb arb merged commit 6c46048 into hapijs:master Sep 27, 2016
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@tribou tribou deleted the tribou:127-boom-internal-docs branch Sep 27, 2016
@hueniverse hueniverse added this to the 4.1.0 milestone Sep 27, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants
You can’t perform that action at this time.