allow passing device_id to login method in client #229
Conversation
non-Jedi
left a comment
non-Jedi
left a comment
There was a problem hiding this comment.
Thanks for taking the time to cleanup the login methods while adding the device_id parameter. Really appreciate it. :)
| } | ||
| for key in kwargs: | ||
| content[key] = kwargs[key] | ||
| if kwargs[key]: |
There was a problem hiding this comment.
I'm confused by this. We're already iterating through all keys in kwargs. Under what circumstances would kwargs[key] be falsey?
All of this is fine, but in case you aren't aware (as I wasn't until a few months ago), content.update(kwargs) will do this on a single line with no explicit iteration required.
There was a problem hiding this comment.
Now reading further on, I see why since we're not validating input in the client method. In light of that, this is fine IMO.
matrix_client/client.py
Outdated
| return self.token | ||
|
|
||
| def login_with_password(self, username, password, limit=10): | ||
| return self.login_with_password(username, password, sync=False) |
There was a problem hiding this comment.
Please include a deprecation warning. :)
| Raises: | ||
| MatrixRequestError | ||
| """ | ||
| token = self.login_with_password_no_sync(username, password) |
There was a problem hiding this comment.
Let's stick a deprecation warning here, too, and move all this functionality to a vanilla login method. Eventually both username and password will be kwargs and an additional kwarg token will be introduced to allow m.login.token with this same method.
Also deprecate the old methods.
|
done |
|
Thanks @Zil0. Pushing a change with a minor tweak to the wording, and then merging this. |
2 participants
Signed-off-by: Valentin Deniaud <valentin.deniaud@inpt.fr>