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 #116: make existing hook take place before the handshake #202

Closed

Conversation

cjerdonek
Copy link
Collaborator

This is an illustration of what I proposed in this comment to add a "pre-handshake" hook. All of the tests still pass (at least on my machine with 3.6.1).

If this approach is acceptable, I could make needed documentation changes, etc, and add a test of using the hook.

@cjerdonek cjerdonek changed the title Proposal for issue #116 (pre-handshake hook) Address issue #116 (pre-handshake hook) Jul 14, 2017
@cjerdonek
Copy link
Collaborator Author

Okay, I added a test and also checked that the test in PR #208 passes, too. For the documentation changes (which would be small), it would be best to wait until after PR #199 is committed IMO. Otherwise, it will create duplicated work.

@cjerdonek cjerdonek force-pushed the issue-116-handshake-hook branch from 45ab1d9 to e83b826 Compare July 19, 2017 00:54
@cjerdonek
Copy link
Collaborator Author

Rebased (still need to address coverage).

@cjerdonek cjerdonek force-pushed the issue-116-handshake-hook branch from b5b6ead to 1c8362f Compare July 22, 2017 02:52
@cjerdonek
Copy link
Collaborator Author

Rebased, and brought coverage back to 100%.

@cjerdonek
Copy link
Collaborator Author

Rebased.

@cjerdonek cjerdonek force-pushed the issue-116-handshake-hook branch from 5515d26 to 02b5a29 Compare July 30, 2017 09:21
@cjerdonek
Copy link
Collaborator Author

Rebased after PR #236.

@cjerdonek cjerdonek changed the title Address issue #116 (pre-handshake hook) Address issue #116: make existing hook take place before the handshake Aug 4, 2017
aaugustin added a commit that referenced this pull request Aug 12, 2017
This replaces the get_response_status() API which never made it into a
release (so there's no backwards incompatibility).

Remove a test that depends on get_response_status() being called after
check_request(). The extension point must be before check_request() so
it can handle regular HTTP requests.

Fix #116.

Supersedes #202 #154, #137.
@aaugustin
Copy link
Member

Thanks for highlighting the drawbacks of #154. Indeed the hook must be before check_request().

I'm proposing a slightly more straightforward approach in #248, which merges get_status_response() and pre_handshake() into a single extension point.

Tentatively closing in favor of #248.

@aaugustin aaugustin closed this Aug 12, 2017
aaugustin added a commit that referenced this pull request Aug 20, 2017
This replaces the get_response_status() API which never made it into a
release (so there's no backwards incompatibility).

Remove a test that depends on get_response_status() being called after
check_request(). The extension point must be before check_request() so
it can handle regular HTTP requests.

Fix #116.

Supersedes #202 #154, #137.
aaugustin added a commit that referenced this pull request Aug 20, 2017
This replaces the get_response_status() API which never made it into a
release (so there's no backwards incompatibility).

Remove a test that depends on get_response_status() being called after
check_request(). The extension point must be before check_request() so
it can handle regular HTTP requests.

Fix #116.

Supersedes #202 #154, #137.
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