Added Member Class Information retrival#112
Conversation
PenguinBoi12
left a comment
PenguinBoi12
left a comment
There was a problem hiding this comment.
The foundation of the PR is solid, it's going to be much easier to interact with users. The code in general is clean but there's a few issues with the tests and calls.
Some of those methods are doing request to matrix, like get_displayname, get_profile, etc, but there's not response validation, so if an error happens it's just going to explode. There's a helper in api called matrix_call that helps with that.
There's cases, like profile, where it might change the behaviour, if you still want to return a default value, although maybe it's better to raise (idk), you can try/catch it.
Additionally, your tests don't seem to have those possible failure covered.
There was a problem hiding this comment.
The name of the file is a bit inaccurate, I think it should be something like member_info.py or just member.py
| def user_id(self) -> str: | ||
| return self._user_id | ||
|
|
||
| async def get_profile(self) -> dict | None: |
There was a problem hiding this comment.
It would be cleaner to have a dataclass for the MemberProfile instead of returning a plain dictionary; it would give callers type safety and make the fields explicit.
| presence = await self._client.get_presence(self._user_id) | ||
| return presence.presence if presence else None | ||
|
|
||
| async def has_permission(self, room: Room, permission: str) -> bool: |
There was a problem hiding this comment.
To be consistent with the following method, I think we could rename this has_room_permission
| avatar = await self._client.get_avatar(self._user_id) | ||
| return await self._client.mxc_to_http(avatar.avatar_url) if avatar else None | ||
|
|
||
| async def get_room_power_level(self, room: Room) -> int: |
There was a problem hiding this comment.
I think you should move this closer (just above them) to has_permission and has_event_permission since they rely on this
| print(f"Can send messages: {can_send_message}") | ||
| ``` | ||
| """ | ||
| power_level = await self.get_room_power_level(room) |
There was a problem hiding this comment.
Just to be sure, does event really use room power level too?
There was a problem hiding this comment.
While looking at this, I think one of the most interesting addition to member you could've made is mention method. Something like:
def mention(self) -> str:
"""Get a Markdown-formatted mention link (matrix.to pill) for this member.
## Example
```python
await ctx.reply(f"Welcome {member.mention()}!")
```
"""
return f"[{self._user_id}](https://matrix.to/#/{self._user_id})"
def __str__(self) -> str:
return self.mention()You can see that I also implemented __str__, I think it's a good idea to make it fallback to mention so e can do:
await ctx.reply(f"Welcome {member}!")But this could be another PR.
| ) | ||
|
|
||
| content = getattr(power_levels_event, "content", {}) or {} | ||
| required_level = content.get(permission, 0) |
There was a problem hiding this comment.
This fallback would cause the power level to always be true.
For example, if the response is an error, it will fallback to 0. This could give access to someone when it shouldn't and it's going to be hard to debug because it's doing it silently. In the worst cases, a permission check should never default to "allowed".
If you use matrix_call it will raise and at the same time default to a "not allowed" behaviour.
Also, there was no test covering that and same thing for has_event_permission
2 participants
Description
Add a command to retrieve member information and a new Member class to manage member-related functionalities.
close #102
Type of Change
Pre-merge Checklist
pytestmypyblack .