use tls_set() in addition to tls_context #126
Conversation
| # TLS set parameter class | ||
| class TLSParameters: | ||
| def __init__( | ||
| self, |
There was a problem hiding this comment.
Change to __init__(self, *, ca_certs, ...). This way, we force the user to supply the keyword argument, which is a good thing IMHO when there are this many options. 👍
There was a problem hiding this comment.
Since there are default values to these parameters (even in paho-mqtt), then is it okay to add * which will force the user to give the values even if it is none? I looked up the usage of * in function calls, which forces the user to give value to an argument that has no default value.
There was a problem hiding this comment.
Yes, the * simply forces the user to use keyword arguments instead of positional arguments. All other semantics stay the same (including defaults). 👍
There was a problem hiding this comment.
Oh makes sense. Added and committed. Please review it :)
asyncio_mqtt/client.py
Outdated
| ca_certs: Optional[str] = None, | ||
| certfile: Optional[str] = None, | ||
| keyfile: Optional[str] = None, | ||
| cert_reqs: Optional[ssl.VerifyMode] = ssl.CERT_REQUIRED, |
There was a problem hiding this comment.
Also, one more change I just saw in paho-mqtt is that I dont have to provide the default values, these should be None as well. So I will change this as well
There was a problem hiding this comment.
Well spotted. 👍 I missed that one.
|
All that's left is the |
asyncio_mqtt/client.py
Outdated
| @@ -117,6 +139,7 @@ def __init__( | |||
| logger: Optional[logging.Logger] = None, | |||
| client_id: Optional[str] = None, | |||
| tls_context: Optional[ssl.SSLContext] = None, | |||
| tls_set_params: Optional[TLSParameters] = None, | |||
There was a problem hiding this comment.
One last nitpick here, and then I promise it's good to merge. 😅 Can we change the name of the parameter from tls_set_params to simply tls_params? Thanks.
There was a problem hiding this comment.
Of course. That makes more sense now that I think of it
|
Thank you for your patience and thank you for your contribution to asyncio-mqtt. 👍 |
|
Learned a lot and thank you for the library. It is a great help to so many :) |
I use the comments about the design of adding
tls_settoasyncio-mqttfrom #14 (comment) and try to implement in the light of that.My implementation allows the use of a
TLSParametersobject to invoketls_set()function of paho-mqtt client in asyncio-mqtt. Let me know if it makes sense. I would also like some pointers in regards to testing this