Added a getStatusCode method. #28
Conversation
o0Ignition0o
force-pushed
the
master
branch
2 times, most recently
from
cc507cf to
528aca9
Compare
|
Can we please merge this or PR29 to get the typescript enums? |
|
My only issue with this change is that currently this project is not compiled. This means using features like |
|
Oh I understand, do you think using a for loop instead is the way to go ? if so i can edit it today :) |
|
Using a for loop + rebased as required :) |
|
This looks better. We still need to add this to the TS definition file, and probably some mention in the README as well. |
|
Oh yeah sure, I will add it today. Thanks a lot for providing a bit of
mentoring despite a tight schedule !
…On Thu, Mar 7, 2019, 23:53 Bryce Neal ***@***.***> wrote:
This looks better. We still need to add this to the TS definition file,
and probably some mention in the README as well.
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#28 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AJBvTq_L8M-E-iSTcqSWfhu__1h6iV1iks5vUZhlgaJpZM4ZAwcA>
.
|
README.md
Outdated
| @@ -26,6 +26,9 @@ response | |||
| .send({ | |||
| error: HttpStatus.getStatusText(HttpStatus.INTERNAL_SERVER_ERROR) | |||
| }); | |||
|
|
|||
| response.status(HttpStatus.getStatusCode('No Content')) | |||
There was a problem hiding this comment.
Can you format these with the line breaks like the others?
README.md
Outdated
| @@ -107,14 +110,22 @@ response | |||
| .send({ | |||
| error: HttpStatus.getStatusText(HttpStatus.INTERNAL_SERVER_ERROR) | |||
| }) | |||
|
|
|||
| response | |||
| .status(getStatusCode('No Content') | |||
There was a problem hiding this comment.
How about we just use internal server error instead of no content like the example above?
| @@ -384,5 +384,13 @@ export declare enum HTTP_STATUS { | |||
| * Convert the numeric status code to its appropriate title. | |||
| * @param statusCode One of the available status codes in this package | |||
| * @returns {String} The associated title of the passed status code | |||
| * @throws {Error} The status code does not exist | |||
index.d.ts
Outdated
| * Convert the status code title to its appropriate numeric value | ||
| * @param statusText One of the available status texts in this package | ||
| * @returns {Number} The associated status code of the passed status text | ||
| * @throws {Error} The status text does not exist |
There was a problem hiding this comment.
I think you forgot to close this comment.
index.d.ts
Outdated
| */ | ||
| export declare function getStatusText(statusCode: number): string; | ||
|
|
||
| /** | ||
| * Convert the status code title to its appropriate numeric value |
There was a problem hiding this comment.
I have some problem with the word "title" in these comments. We should refer to them consistently as "status code" and "status text".
In fact the IETF spec calls it a "Reason Phrase" so ideally we could change all of the comments from "title" -> "reason phrase".
The reason-phrase element exists for the sole purpose of providing a
textual description associated with the numeric status code, mostly
out of deference to earlier Internet application protocols that were
more frequently used with interactive text clients. A client SHOULD
ignore the reason-phrase content.
|
Thanks a lot for such a thorough code review. I think I have addressed most of the concerns :) I chose not to rename the getStatusText function name to getReasonPhrase because that would be a breaking change, but I can change it if you want to :) By the way I was skimming over the HTTP status codes and it seems like the 500 Reason phrase is "Internal Server Error" whereas we here refer to it as "Server Error". As stated in the IETF spec, a client should ignore the reason-phrasse content so it's probably not a big deal, plus we probably wouldn't want to cause a breaking change ^^ |
|
Can we merge this now please? |
|
Sorry for the delay. |
this should fix #27
Please let me know if there's anything I can do to improve the pull request :)