Feat: Downgrade to 1.1 #3576
Feat: Downgrade to 1.1 #3576
Conversation
I don't think this is more readable but who am I to argue with deepsource.
Words hard.
Bibo-Joshi
left a comment
There was a problem hiding this comment.
Thanks for the PR!
I decided to go for warning when a custom http://, not https:// URL is set.
IISC the logic for the warnings-checking is not yet covering everything. After reviewing your changes, the different scenarios (all considering the user passed http_version="2") that I see are:
- User is using cloud api -> base url starts with https://,
local_mode is False, everything should work - User is using a self-hosted, non-local API server, with or without HTTP/1.1 proxy -> base_url can be either http://… or https://…,
local_mode is False, will not work due to HTTP/2 - User is using a self-hosted, non-local API server with a HTTP/2 proxy -> base_url can be either http://… or https://…,
local_mode is False, will work due to proxy - User is using a self-hosted local API server with or without a HTTP/1.1 proxy -> base_url is http://…,
local_mode is True, will not work due to HTTP/2 - User is using a self-hosted local API server with a HTTP/2 proxy -> base_url can be http://… or https://…,
local_mode is True, will not work due to proxy
In summary all combinations of http(s) & local_mode=True/False can either work or not work. IMO we should just issue a warning as soon as we know that the user is using a self hosted API server. This will give a few false positives, but it's only a warning after all …
I guess the only reliable way to check if the user is using a self-hosted API server is to check if "://api.telegram.org/" in base_(file_)url …
I also decided to call the next version 20.1.1, I think this issue warrants a release since users face errors because of us which are hard to spot/track down in production.
Fine with me 👍
I also decided to change away the [http2] dependency, I hope this doesn't break anything.
Please make it a proper optional dependency via requirements-opts.txt and also add unit test for that :)
|
@Bibo-Joshi You mention |
|
@Poolitzer good point. |
Bibo-Joshi
left a comment
There was a problem hiding this comment.
Thanks for the updates! The warning logic now LGTM :)
Co-authored-by: Bibo-Joshi <22366557+Bibo-Joshi@users.noreply.github.com>
Also broke the warning logic wuuups
Co-authored-by: Bibo-Joshi <22366557+Bibo-Joshi@users.noreply.github.com>
Bibo-Joshi
left a comment
There was a problem hiding this comment.
LGTM! If there's nothing left from your side, we can merge IMO :)
|
@Bibo-Joshi Nothing from me |
harshil21
left a comment
There was a problem hiding this comment.
looks good, just 2 test class renaming suggestions..
Co-authored-by: Harshil <37377066+harshil21@users.noreply.github.com>
I decided to go for warning when a custom http://, not https:// URL is set. I also decided to call the next version 20.1.1, I think this issue warrants a release since users face errors because of us which are hard to spot/track down in production.
I also decided to change away the [http2] dependency, I hope this doesn't break anything.