-
Notifications
You must be signed in to change notification settings - Fork 78
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
paho.mqtt.proxy_set() - support #127
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for opening this PR. 👍 Looks good to me. I only have one minor comment about typing (see the review).
asyncio_mqtt/client.py
Outdated
def __init__( | ||
self, | ||
*, | ||
proxy_type: int = None, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You forgot the Optional
part here. :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you look at the description here: https://github.com/eclipse/paho.mqtt.python/blob/225ab3757f6818ba85eb80564948d1c787190cba/src/paho/mqtt/client.py#L875
It says
(Required)
proxy_type: One of {socks.HTTP, socks.SOCKS4, or socks.SOCKS5}
proxy_addr: IP address or DNS name of proxy server
(Optional)
proxy_rdns: boolean indicating whether proxy lookup should be performed
remotely (True, default) or locally (False)
proxy_username: username for SOCKS5 proxy, or userid for SOCKS4 proxy
proxy_password: password for SOCKS5 proxy
In line with that shouldnt it be like this, or should I change it to Optional?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also do you think it is a good idea to add some documentation so that the users know that the functionality exists (e.g a small example) in the README.md in another PR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, good point. 👍 If those arguments are required then let's not use default valures (= None
) for them.
Looking at the paho implementation, we could have a signature like this:
def __init__(
self,
*,
proxy_type: int,
proxy_addr: str,
proxy_rdns: Optional[bool] = True,
proxy_username: Optional[str] = None,
proxy_password: Optional[str] = None
): ...
This way, the user must supply the proxy_type
and proxy_addr
arguments.
Also do you think it is a good idea to add some documentation so that the users know that the functionality exists (e.g a small example) in the README.md in another PR?
Yes, that's a good idea. 👍
Specifically, we should let people know that this whole proxy is an "extra" feature (even in paho) that requires the PySocks
dependency.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have fixed it and committed it as well.
Alright. I will work on the documentation and create a PR for that
LGTM and merged. 👍 Thank you for your contribution to asyncio-mqtt (again). :) |
Thank you for the opportunity (again) :D |
This commit adds the functionality of paho-mqtt's
proxy_set()
to asyncio-mqtt