Api devices #209
Api devices #209
Conversation
|
In general, tests that are relatively "non-magical" are better even if they aren't very DRY in my opinion. Then again, responses already feels pretty magical. I'm pretty ambivalent other than generally feeling that we need to make sure we don't abstract too far away from what we're actually doing in the test code. @pik 's stuff testing explicitly against the spec I'm definitely planning on looking at eventually, but I don't think we should replace manually written tests with that unless we set something up where we explicitly declare each method of |
non-Jedi
left a comment
non-Jedi
left a comment
There was a problem hiding this comment.
Other than nits, LGTM. Thanks!
|
|
||
| def update_device_info(self, device_id, display_name): | ||
| """Update the display name of a device. | ||
| Args: |
There was a problem hiding this comment.
Please include a blank line after every first line summary of a docstring.
matrix_client/api.py
Outdated
|
|
||
| def delete_device(self, auth_body, device_id): | ||
| """Deletes the given device, and invalidates any access token associated with it. | ||
| Requires Auth. |
There was a problem hiding this comment.
Should probably explicitly say that it uses the "User-Interactive Authentication API." Lots of endpoints require auth, but very few use the user-interactive flow. Also should be separated from the "Args" block by an empty line I believe.
| @responses.activate | ||
| def test_delete_device(self): | ||
| delete_device_url = "http://example.com/_matrix/client/r0/devices/QBUAZIFURK" | ||
| responses.add(responses.DELETE, delete_device_url, body='{}') |
There was a problem hiding this comment.
Can we test that things behave as expected when receiving 401 responses since that's part of the user-interactive auth api?
|
Oh, and signoff please. :) |
|
Should be good now. |
matrix_client/api.py
Outdated
|
|
||
| def delete_device(self, auth_body, device_id): | ||
| """Deletes the given device, and invalidates any access token associated with it. | ||
| Requires User-Interactive Authentication API. |
There was a problem hiding this comment.
Sorry I missed this the first time through. Please try to follow PEP 257 conventions for multi-line docstrings. In this case, that means a single summary line separated by a blank new line from a more detailed description. Something like:
"""Deletes the given device and invalidates any associated access tokens.
NOTE: This endpoint uses the User-Interactive Authentication API.
Args:
auth_body (dict): Authentication params.
device_id (str): The device ID of the device to delete.
"""There was a problem hiding this comment.
Thanks, fixed. I mostly know PEP8 through flake8 and tend to miss those kind of things.
3 participants
Not sure if I should squash pik's commit and my fixes.
Tests felt really repetitive to write, I wonder if there isn't some way to refactor them (maybe using some pytest magic). edit: just saw that there have already been discussions on this and that pik had started working on the issue, I should see if I can take the time to look into this.