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

parse hmsg with status code and description #125

Merged
merged 1 commit into from
May 17, 2022
Merged

Conversation

mmmries
Copy link
Collaborator

@mmmries mmmries commented May 16, 2022

This fixes the bug from #122

I got the parsec working correctly here, but I think we should also return the status code and/or description back to the recipient. This PR currently throws that data away, but especially in the case of the pull request with no wait, it seems very useful to let the caller know that we got back a 404 vs a 503 or some other status code.

@mmmries
Copy link
Collaborator Author

mmmries commented May 16, 2022

@brandynbennett curious if you have any thoughts on this?

@brandynbennett
Copy link
Contributor

I agree we should include the status code somehow. I'm less confident about where in the return value to add that

@brandynbennett
Copy link
Contributor

This line from the architecture docs makes me think the error status should be handled differently.

Whenever there is a status message, it is recommended that the error is not returned to the user as a processable message and instead handle is as the error condition

I think that is a separate issue though from #122 and would require a deeper understanding of possible status codes to parse them and handle them appropriately. This PR at least gets the headers parsing without breaking. It makes sense to me to address the status code handling in separate PR/issue

@mmmries
Copy link
Collaborator Author

mmmries commented May 16, 2022

@brandynbennett thanks for the link to that ADR document. I wasn't thinking of a status code of 100 as an error-case, but I'll go back and read that in it's entirety to see if it makes more sense to me.

I agree though, that we should probably ship this as-is and then come back to the handling of the status code. I think that's what you were saying?

@brandynbennett
Copy link
Contributor

Ya I think this PR is good to go, and the status code handling can be a separate PR

@mmmries mmmries merged commit 3bf6eb1 into main May 17, 2022
@mmmries mmmries deleted the fix_header_parsing branch May 17, 2022 08:43
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