-
Notifications
You must be signed in to change notification settings - Fork 16
feat: ✨ trigger the bot by mentioning it #81
base: master
Are you sure you want to change the base?
Conversation
Perhaps create a new issue for this? |
|
Fixed messagematch.mention bug - e73f24b |
"body" is now a tuple instead of a list
f5ec850 to
e73f24b
Compare
|
Accidentally included a change undescribed by commit message, fixed. |
|
2d63cbe should improve readability. Please test if this broke anything. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
introduces an exception, needs to be changed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
smart idea using a loop, somehow I missed that.
however, some changes required
The trouble is that this is required for this PR, hence it's included. |
|
|
||
| resp = await self.async_client.sync(timeout=65536, | ||
| full_state=False) #Ignore prior messages | ||
| full_state=True) #Ignore prior messages |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
seems to be required to reliably load self.room.own_user_id and self.room.users which may be empty otherwise from testing. I hope there is a better way to do it than just syncing everything.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does self.room.own_user_id follow the structure of @username:homeserver ? If so, it would not be neccesary to do anything with self.room.users to obtain it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
self.room is a Dict[str, MatrixUser]. MatrixUser contains display_name and disambiguated_name which we need for mention() matches
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe this https://matrix-nio.readthedocs.io/en/latest/nio.html#nio.rooms.MatrixRoom.user_name is good enough instead? I don't think so as it does the same: if room members haven't been synced yet, it just fails.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we need the user_id, then whoami should solve that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lines 57-63 of api.py
async with aiohttp.ClientSession() as session:
async with session.get(f'{self.creds.homeserver}/_matrix/client/r0/account/whoami?access_token={self.creds.access_token}') as response:
device_id = ast.literal_eval((await response.text()).replace(":false,", ":\"false\","))['device_id']
user_id = ast.literal_eval((await response.text()).replace(":false,", ":\"false\","))['user_id']
self.async_client.device_id, self.creds.device_id = device_id, device_id
self.async_client.user_id, self.creds.user_id = user_id, user_id
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would the user id not be stored in bot.async_client.user_id ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it would. the issue is more about getting the displayname though
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think doing a full sync is actually ok if we enable storage in this PR (store is needed for #79)
then only the very first time would be a "big" sync
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That is acceptable.
838be53 to
2d63cbe
Compare
|
57147ec looks good. |
missing: mention when no prefix is set
happens when calling args before command
even when no prefix is given
Refactor MessageMatch class to use _body_without_mention and _split_body. No longer checks formatted_body for mention.
Fix messagematch tests. match9 and match10 in test_mention are disabled, but will need to be enabled to test switching between body and formatted_body
| """ | ||
|
|
||
| for id in [self._own_disambiguated_name, self._own_display_name, self._own_user_id]: | ||
| for id in [self._own_disambiguated_name, self._own_display_name, self._own_user_id, self._own_display_name_colon]: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this will always match _own_display_name and never _own_display_name_colon
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps _own_display_name_colon could be removed.
|
|
||
| self.mention() # Set self._mention_id_length | ||
| self._body_without_prefix = self.event.body[len(self._prefix):] | ||
| self._body_without_mention = self.event.body[self._mention_id_length:] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why initialize these here while it's unknown whether prefix or mention is used?
why create member variables that are only ever used 2 lines later?
mention() is executed twice unnecessarily
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
mention() sets ._mention_id_length, which is neccesary for calculating ._body_without_mention
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it may be a good idea to look at bot libraries for other networks to see how they handle matching.
| elif self.prefix(): | ||
| body = self._body_without_prefix | ||
| else: | ||
| body = self.event.body |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why use distinct member variables? they can't both apply at once
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you reword this?
|
|
||
| if not body_without_prefix: | ||
| return [] | ||
| if not (self._body_without_prefix and self._body_without_mention): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
use len(self._split_body) instead
…MessageMatch class
a711bef to
e95148d
Compare
example:
works with pills, displayname, disambiguated disaplyname, full matrix ID.
However as-is, it needs a full sync to be able to resolve this (
userscan be empty otherwise):https://github.com/KrazyKirby99999/simple-matrix-bot-lib/blob/33776bad9855c4337557d3badfe3af74e9c9af1a/simplematrixbotlib/match.py#L85
I don't think that's optimal since a full sync on startup can take long when the account/room reached a certain lifetime. Is it possible to sync only the members list of the required room on demand?
todo