Skip to content

Conversation

@jjberry314
Copy link

Added reasonPhraseExtended() to allow retrieval of extended human-readable description text
Added some missing codes

…dable description text

Added some missing code
@j-ulrich j-ulrich self-requested a review October 4, 2018 20:44
Copy link
Owner

@j-ulrich j-ulrich left a comment

Choose a reason for hiding this comment

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

@jjberry314
Thank you for your contribution! I appreciate that!

Please let me know if you don't want to take care of my review comments, then I will do it.

@jjberry314
Copy link
Author

I think, on reflection, my changes can be discarded if you wish. I don't think I need reasonPhraseExtended() at all now. If my other tweaks are useful then please feel free to adopt them, or just reject the pull request.

…ch emits only code + phrase rather than a description
@jjberry314
Copy link
Author

jjberry314 commented Oct 5, 2018

I've replaced reasonPhraseExtended() and replaced it with reasonPhraseEx() which emits "HTTP code: phrase" where a phrase is available or "HTTP code" if not.

I have not addressed your other review comments at this time. I have no strong feeling about keeping those changes, as I don't really need them. I'm happy to revert them.

@j-ulrich
Copy link
Owner

j-ulrich commented Oct 6, 2018

I've replaced reasonPhraseExtended() and replaced it with reasonPhraseEx() which emits "HTTP code: phrase" where a phrase is available or "HTTP code" if not.

Sorry, but IMO the format of that string is application specific and therefore does not belong into the library. Someone else might need such a message to be formatted like "HTTP <code> - <phrase>" or "<phrase> (<code>)" or whatever.

So I will now create a separate pull request to add the missing status codes but leave out the reasonPhraseEx() function.
Let me know if you see it differently.

@jjberry314
Copy link
Author

I completely understand and accept your reasoning.

Thank you for being so patient with me.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants