Skip to content

Allow "Connection established" in check_reason #1

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

Closed
wants to merge 2 commits into from

Conversation

lambdalisue
Copy link
Contributor

First of all, thanks for the library.

I've tried this with proxy.py and I've found that it seems the message is "Connection established" rather than "Connection Established" at least for proxy.py.

https://github.com/abhinavsingh/proxy.py/blob/c09fc8627727fed98dd1bdde8321a6efa36a8fcd/proxy/http/proxy/server.py#L106

Additionally, I wonder why you check the message string itself. I feel it's a bit fragile so could you tell me the reason for this?

@LinkTed
Copy link
Owner

LinkTed commented Jun 3, 2021

Thanks for your contribution.

There is no specific reason, why there is the check. I only want to be sure that the connection was established.

@LinkTed
Copy link
Owner

LinkTed commented Jun 3, 2021

Could you fix the formatting, please?

@lambdalisue
Copy link
Contributor Author

There is no specific reason, why there is the check. I only want to be sure that the connection was established.

It's a bit beyond this PR but can we change the logic? For example, just check the status code.
I don't think it's a good idea to check the message while no one guarantee the equality of the message for individual proxy implementations...

@LinkTed
Copy link
Owner

LinkTed commented Jun 3, 2021

Sure, remove the function check_reason. In the end, please squash the commits.

lambdalisue added a commit to lambdalisue/async-http-proxy that referenced this pull request Jun 4, 2021
@lambdalisue
Copy link
Contributor Author

lambdalisue commented Jun 4, 2021

I made a new PR #2 which remove check_reason. Please check that.

@lambdalisue lambdalisue closed this Jun 4, 2021
@lambdalisue lambdalisue deleted the fix-reason branch June 4, 2021 03:39
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