Conversation
matrix_client/api.py
Outdated
|
|
||
| def key_changes(self, from_token, to_token): | ||
| """Gets a list of users who have updated their device identity keys | ||
| since a previous sync token. |
There was a problem hiding this comment.
Should be a single line summary.
|
Addressed stylistic issues and added tests. |
|
@Zil0 could you rebase on master please to make review easier? |
|
done :) |
non-Jedi
left a comment
non-Jedi
left a comment
There was a problem hiding this comment.
Other than that, LGTM. Thanks!
matrix_client/api.py
Outdated
| } | ||
| return self._send("POST", "/delete_devices", content=content) | ||
|
|
||
| def upload_keys(self, device_keys={}, one_time_keys={}): |
There was a problem hiding this comment.
kwargs shouldn't have mutable objects as their default argument. We do this elsewhere in the sdk, and it's wrong. What effectively happens is that the same dictionary (which is mutable) gets reused each time this method is called and could potentially be non-empty in the future.
matrix_client/api.py
Outdated
| Said device must be the one used when logging in. | ||
|
|
||
| Args: | ||
| device_keys (dict): Optional. Identity keys for the device. |
There was a problem hiding this comment.
Should give info about the format of this arg in the docstring. Same for below.
matrix_client/api.py
Outdated
| content["one_time_keys"] = one_time_keys | ||
| return self._send("POST", "/keys/upload", content=content) | ||
|
|
||
| def query_user_keys(self, user_id): |
There was a problem hiding this comment.
This is a higher level method that I don't think belongs in this class.
There was a problem hiding this comment.
right, same for claim_key then?
| """Query HS for public keys by user and optionally device. | ||
|
|
||
| Args: | ||
| user_devices (dict): The devices whose keys to download. Should be |
There was a problem hiding this comment.
Please format these for rst as seen in other method docstrings where args need multi-line explanations.
matrix_client/api.py
Outdated
| """ | ||
| return self.query_keys({user_id: []}) | ||
|
|
||
| def query_keys(self, user_devices, timeout=None): |
There was a problem hiding this comment.
Where's the token parameter for this method? https://matrix.org/speculator/spec/HEAD/client_server/unstable.html#post-matrix-client-r0-keys-query
There was a problem hiding this comment.
Good catch, I wasn't looking at unstable :)
matrix_client/api.py
Outdated
| """Gets a list of users who have updated their device identity keys. | ||
|
|
||
| Args: | ||
| from_token (str): The desired start point of the list. |
There was a problem hiding this comment.
Please explicitly state that this should be taken from next_batch field of a sync response.
|
|
||
| @responses.activate | ||
| @pytest.mark.parametrize("args", [ | ||
| {}, |
There was a problem hiding this comment.
I don't really entirely understand pytest. Where is this empty dict going? upload_keys only takes two arguments, not three.
There was a problem hiding this comment.
https://docs.pytest.org/en/latest/parametrize.html
The test is called 3 times, one with {} which stands for no argument, then with device_keys set, then with one_time_keys. Not sure why I didn't do a fourth test with both set. Then we call the API method with the arguments found in args.
If this is too confusing for what it is, I can go back to copy-pasting, which won't make much difference seeing the rest of the file :)
|
Making the changes but before I forget again: |
along with some helpers
|
fixed :) |
|
LGTM. Thanks! |
2 participants
Follows #209 and is based on the work of @pik at pik@f855cb8.
This can already be looked at, I tagged it WIP because I didn't add tests yet.