Add first group support #179
Conversation
|
@tinloaf Thanks! This has been added to my todo list. Any chance of some tests at least for the non- |
|
Yes, that's on my todo list. ;-) However, since the API is not yet really well-documented, writing the tests according to the (non-existent) Spec is somewhat difficult. I actually wondered whether it would be possible to have Travis install and spin up a fresh synapse installation for every test, and then test against synapse… This would certainly slow testing down, but it would make everything testable. For local (i.e., non-travis) tests, you would have to start your own synapse instance then… But maybe those tests could be optional? |
|
@non-Jedi I've added tests (well, if you want to call them that…) for the |
|
Heh. I haven't looked at it closely yet, but you're thinking of it exactly I think in general we should make an effort to test even parts of the sdk using |
matrix_client/api.py
Outdated
| | group. | ||
| group_id (str): The group ID | ||
| """ | ||
| self._send("POST", "/groups/{}/profile".format(quote(group_id)), |
There was a problem hiding this comment.
please add a return here. see #167
matrix_client/api.py
Outdated
| quote(group_id))) | ||
|
|
||
| def publicise_group(self, group_id, make_public): | ||
| """Leave a group. |
| body = { | ||
| "localpart": localpart | ||
| } | ||
| return self._send("POST", "/create_group", body) |
There was a problem hiding this comment.
All apis that haven't made it into the official spec yet should be called under the "unstable" api prefix. Believe it is /_matrix/client/unstable.
| """Update the profile of a group. | ||
|
|
||
| Args: | ||
| profile_data (dict): The request payload. |
There was a problem hiding this comment.
Any reason not to make this 4 separate arguments? name, avatar_url, short_description, and long_description?
|
|
||
| | name (string): The new name of the group | ||
|
|
||
| | avatar_url (URL): A URL pointing to the new URL for the |
There was a problem hiding this comment.
Probably should explicitly state that this is an mxc url.
| @@ -290,6 +309,21 @@ def get_rooms(self): | |||
| """ | |||
| return self.rooms | |||
|
|
|||
| def get_groups(self): | |||
There was a problem hiding this comment.
Why is the approach different here than for rooms? Can't we just make it a requirement that get_groups returns nothing meaningful if a sync hasn't yet completed?
If so, we shouldn't introduce this method at all since we're currently deprecating get_rooms.
There was a problem hiding this comment.
Just read through the groups docs, and I see that they don't specify anywhere
what groups stuff looks like coming down /sync. Nevertheless, group info does
come down /sync, and looks like:
{'leave': {},
'join': {},
'invite': {'+test:thebeckmeyers.xyz': {'profile': {'avatar_url': None,
'name': 'test'},
'inviter': '@adam:thebeckmeyers.xyz'}}}Even though it isn't written down anywhere, I'd like to use the info from
/sync rather than introducing a completely separate polling function. We'll
just have to revisit if this changes in the future.
| [ user_id ]: List of user IDs of the users in the group. | ||
| """ | ||
| response = self.client.api.get_users_in_group(self.group_id) | ||
| return [event["user_id"] for event in response["chunk"]] |
There was a problem hiding this comment.
Could we (should we?) return a list of User objects instead of a list of strings here (and elsewhere in class)?
| Returns: | ||
| boolean: True if the room was added. | ||
| """ | ||
| if isinstance(room_id, Room): |
There was a problem hiding this comment.
Should we mention that argument can be Room object as well as string in docstring? Should we make methods that take a user_id argument function in the same way with User objects?
| else: | ||
| print("Check your server details are correct.") | ||
| sys.exit(2) | ||
| except MissingSchema as e: |
There was a problem hiding this comment.
We should be catching this error from requests since requests is supposed to be an implementation detail. I'll file an issue...
| response = self.client.api.get_rooms_in_group(self.group_id) | ||
| return [event["room_id"] for event in response["chunk"]] | ||
|
|
||
| @property |
There was a problem hiding this comment.
Since we're already doing this with some fields, why not change the get_rooms method to rooms and add a @property decorator?
This adds the first elements of the new groups API to the SDK. A couple of things:
Major Caveat: The question of which changes in groups cause an event to appear at the "/sync" API endpoint still seems to be unclear. Therefore, the current implementation does not use events at all, but polls the API endpoints every time some information is requested. I've but big warnings into the docstrings that the respective methods do not cache, and that you should not overuse them.
I've also put warnings everywhere that the API is still unstable and may break at any time.