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 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
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
Copy link
Contributor

Marsup commented Sep 26, 2016

@arb you authorized it in #127 :D

@arb
Copy link
Contributor

arb commented Sep 26, 2016

That was the old me.

@mtharrison
Copy link
Contributor

@tribou
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

@Marsup
Copy link
Contributor

Marsup commented Sep 26, 2016

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

@tribou
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
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
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
@@ -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`
Copy link
Contributor

Choose a reason for hiding this comment

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

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

@@ -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)
Copy link
Contributor

Choose a reason for hiding this comment

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

Undo this please. The TOC is automatically generated.

@tribou tribou force-pushed the 127-boom-internal-docs branch from 8a65bfb to 1cd79ad Compare September 27, 2016 02:30
@tribou
Copy link
Contributor Author

tribou commented Sep 27, 2016

Both changes should now be updated.

@arb arb self-assigned this Sep 27, 2016
@arb arb added the documentation Non-code related changes label Sep 27, 2016
@arb arb merged commit 6c46048 into hapijs:master Sep 27, 2016
@tribou tribou deleted the 127-boom-internal-docs branch September 27, 2016 13:03
@hueniverse hueniverse added this to the 4.1.0 milestone Sep 27, 2016
@lock
Copy link

lock bot commented Jan 9, 2020

This thread has been automatically locked due to inactivity. Please open a new issue for related bugs or questions following the new issue template instructions.

@lock lock bot locked as resolved and limited conversation to collaborators Jan 9, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
documentation Non-code related changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants