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

Address issue #197 re: handshake customization docs. #199

Closed

Conversation

cjerdonek
Copy link
Collaborator

Addresses #197.

``HTTPStatus.UNAUTHORIZED`` or ``HTTPStatus.FORBIDDEN``.
``HTTPStatus.UNAUTHORIZED`` or ``HTTPStatus.FORBIDDEN``. The
current request path can be accessed from within this method
as ``self.path``.
Copy link
Member

Choose a reason for hiding this comment

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

It may be useful to describe how to access headers (request_headers, raw_request_headers) as well, which doesn't quite belong to a docstring.

Perhaps it would be best to add a section to the docs about customizing the handshake process and give all the details there. What do you think?

(I also need to look at the other related issue.)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

A section like that in the docs makes sense, though I think I'd like to wait until after #116 is resolved to do that since it may require rewriting a portion those docs.

I do think the fact that self.path and self.request_headers are guaranteed to be available should be stated in the same place developers are told the method is overridable (currently stated in the docstring). That way implementers are clearer on what they can / should be relying on when they override.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Note to self: self.origin is also available.

@cjerdonek
Copy link
Collaborator Author

Patch updated based on feedback.

@cjerdonek cjerdonek force-pushed the issue-197-handshake-docs branch from 488033b to 49e6e26 Compare July 10, 2017 14:35
@aaugustin
Copy link
Member

This looks pretty good, thanks a lot for your work.

I'll take the time to review next week-end (hopefully).

@cjerdonek cjerdonek force-pushed the issue-197-handshake-docs branch from e62c9a3 to bd809d7 Compare July 30, 2017 09:27
@cjerdonek
Copy link
Collaborator Author

Rebased after PR #236.

@aaugustin
Copy link
Member

This needs updating after #248.

@aaugustin
Copy link
Member

I'm closing this PR because it's out of date. The corresponding issue #197 is still open.

@aaugustin aaugustin closed this Nov 1, 2017
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