-
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. |
@@ -39,7 +39,7 @@ async def main(self): | |||
|
|||
|
|||
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
@@ -149,11 +155,12 @@ def mention(self): | |||
Returns True if the message begins with the bot's username, MXID, or pill targeting the MXID, and False otherwise. | |||
""" | |||
|
|||
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 (
users
can 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