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

Supporting sending replies #27

Merged
merged 7 commits into from
Sep 22, 2015

Conversation

justinvdm
Copy link
Contributor

This and #26 will make replying possible.

@justinvdm
Copy link
Contributor Author

Ready for review.

'''Raised when a message is not found.'''
name = 'MessageNotFound'
description = 'message not found'
code = http.NOT_FOUND
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure if this is the right code to use. I think 404 responses are usually for telling the client that the requested entity was not found, not that something needed to fulfill the request was not found.

Copy link
Contributor

Choose a reason for hiding this comment

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

So the action is "reply to this message" and we're saying "that message doesn't exist". So I would say it's a 400 Bad Request, but I think it would be good to have a discussion on this.

Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a reason why this is in stores, but not used in stores and used in channel?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

+1 on 400.

Ya, wasn't too sure where to put the error. I put it in stores in case something else irrelevant to channel was using stores needed to raise such an error. Happy to move it into channel though.

Might be a sign that we need to raise an error in the stores methods (counter to what I suggested in an earlier PR review), but not too sure, still might seem weird to have some of our methods throwing errors and some returning None.

Copy link
Contributor

Choose a reason for hiding this comment

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

It might also be a bit strange having the store raise a 400 error if it can't find something.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looking at the spec, 400 looks appropriate:

The 400 (Bad Request) status code indicates that the server cannot or will not process the request due to something that is perceived to be a client error (e.g., malformed request syntax, invalid request message framing, or deceptive request routing)

Rather than

The 404 (Not Found) status code indicates that the origin server did not find a current representation for the target resource or is not willing to disclose that one exists.

Will go with 404 and move the error to channel for now.

@rudigiesler
Copy link
Contributor

Just those two comments. Looking good to me otherwise.

@justinvdm
Copy link
Contributor Author

Ready for re-review.

@rudigiesler
Copy link
Contributor

👍

@justinvdm justinvdm merged commit 717ec56 into develop Sep 22, 2015
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