4
31
Fork
You've already forked simplematrixbotlib
21

v3 API #91

Open
charlhakeem wants to merge 10 commits from charlhakeem/simplematrixbotlib:v3 into v3
pull from: charlhakeem/simplematrixbotlib:v3
merge into: imbev:v3
imbev:master
imbev:v3
imbev:send_reaction
Contributor
Copy link

Fixes #62

Fixes #62
@ -29,4 +36,1 @@
content["m.relates_to"]
except KeyError:
content["m.relates_to"] = {} # type: ignore
content["m.relates_to"]["m.in_reply_to"] = { # type: ignore
Owner
Copy link

Won't this cause a KeyError?

Won't this cause a KeyError?
charlhakeem marked this conversation as resolved
Author
Contributor
Copy link

Is the v3 branch currently suitable for running a basic bot for development purposes? The Bot class appears to be incomplete, and example bots fail to start.

Is the v3 branch currently suitable for running a basic bot for development purposes? The Bot class appears to be incomplete, and example bots fail to start.
Owner
Copy link

Is the v3 branch currently suitable for running a basic bot for development purposes? The Bot class appears to be incomplete, and example bots fail to start.

Yes, it is suitable, though incomplete. The examples are from version 2 and out of date. See the README.md for an updated example.

> Is the v3 branch currently suitable for running a basic bot for development purposes? The Bot class appears to be incomplete, and example bots fail to start. Yes, it is suitable, though incomplete. The examples are from version 2 and out of date. See the README.md for an updated example.
charlhakeem changed title from (削除) Add markdown to v3 (削除ここまで) to Add markdown and send_media to v3 2024年08月03日 22:22:57 +02:00
@ -27,2 +73,3 @@
})
if reply_to_event_id:
try:
Owner
Copy link

This approach is easier to extend , e.g. for threads. Please revert this chunk

This approach is easier to extend , e.g. for threads. Please revert this chunk
charlhakeem marked this conversation as resolved
@ -20,3 +58,2 @@
async def send_text(self, message_body: str, reply_to_event_id: Optional[str] = None):
async def send_text(self, message_body: str, _markdown: Optional[bool] = False, reply_to_event_id: Optional[str] = None):
Owner
Copy link

Can you change you replace _markdown with format_as_markdown: bool ?

Can you change you replace _markdown with `format_as_markdown: bool` ?
charlhakeem marked this conversation as resolved
@ -27,0 +66,4 @@
md = markdown.markdown(message_body,
extensions=["sane_lists", "fenced_code", "nl2br"])
content["body"] = strip_html(md)
Owner
Copy link

What is the purpose of stripping html?

What is the purpose of stripping html?
Author
Contributor
Copy link

The body field should only contain unformatted plain text, with all formatting and styling reserved for the formatted_body field.

The `body` field should only contain unformatted plain text, with all formatting and styling reserved for the `formatted_body` field.
Owner
Copy link

Does that break html codeblocks? e.g.

"""
text here

<p>my html</p>

more text
"""

Does that break html codeblocks? e.g. """ text here ```html <p>my html</p> ``` more text """
Author
Contributor
Copy link

I've reverted the HTML stripping, as it seemed unnecessary and was causing issues. The current behavior should now align with the v2 version.

I've reverted the HTML stripping, as it seemed unnecessary and was causing issues. The current behavior should now align with the v2 version.
charlhakeem marked this conversation as resolved
@ -41,0 +114,4 @@
"url": resp.content_uri
}
if not filetype == "file":
Owner
Copy link

Checking for each file type instead of using guards would be more appropriate in this case.

Checking for each file type instead of using guards would be more appropriate in this case.
charlhakeem marked this conversation as resolved
charlhakeem changed title from (削除) Add markdown and send_media to v3 (削除ここまで) to Add markdown, contains, and send_media to v3 2024年08月04日 22:34:54 +02:00
charlhakeem changed title from (削除) Add markdown, contains, and send_media to v3 (削除ここまで) to v3 API 2024年08月05日 22:20:58 +02:00
@ -14,2 +15,4 @@
def __repr__(self):
return self.body
def get_body(self):
Owner
Copy link

Formatted body and body should be decoupled. The user should have an easy way to fetch the body, a way to check if the body is formatted, and a way to get the formatted body.

Instead of get_body(), I suggest a get_formatted_body method. This method should have a parameter of something like with_reply_markup, determining whether or not to include . If there is no formatted body, it should return None.

Formatted body and body should be decoupled. The user should have an easy way to fetch the body, a way to check if the body is formatted, and a way to get the formatted body. Instead of get_body(), I suggest a get_formatted_body method. This method should have a parameter of something like `with_reply_markup`, determining whether or not to include <mx-reply>. If there is no formatted body, it should return None.
Author
Contributor
Copy link

This method is intended for scenarios where the user requires quick access to a message's text content. If the raw text is needed, user can simply retrieve message.body or message.formatted_body without utilizing this method.

This method is intended for scenarios where the user requires quick access to a message's text content. If the raw text is needed, user can simply retrieve `message.body` or `message.formatted_body` without utilizing this method.
@ -16,0 +20,4 @@
return re.sub(r'<mx-reply>.*?</mx-reply>', '', self.formatted_body)
return self.body
def command(self, command: str, case_sensitive: bool = True):
Owner
Copy link

Version 3 will not support a command method.

It was unreliable with some messages, and barely an improvement over str.startswith()

Version 3 will not support a command method. It was unreliable with some messages, and barely an improvement over `str.startswith()`
charlhakeem marked this conversation as resolved
@ -16,0 +31,4 @@
if regex:
return bool(re.search(string, body, 0 if case_sensitive else re.IGNORECASE))
if not isinstance(string, str):
Owner
Copy link

string should only be a string. If the library user needs to check multiple strings, they can make multiple calls.

If needed, accepted types can be expanded at a later date.

string should only be a string. If the library user needs to check multiple strings, they can make multiple calls. If needed, accepted types can be expanded at a later date.
Author
Contributor
Copy link

Removing support for Iterables would reduce the usefulness of the contains method. It is easier and more readable to pass a predefined list of words directly to contains for matching than to manually create a for loop and call the method in each iteration.

Removing support for Iterables would reduce the usefulness of the `contains` method. It is easier and more readable to pass a predefined list of words directly to `contains` for matching than to manually create a `for` loop and call the method in each iteration.
@ -18,3 +40,4 @@
def __repr__(self):
return f"{self.room_id}: {self.name}"
async def send_text(self, message_body: str, format_as_markdown: bool = False, reply_to_event_id: Optional[str] = None, room_id: Optional[str] = None):
Owner
Copy link

Can you add the appropriate response/error return type?

Can you add the appropriate response/error return type?
charlhakeem marked this conversation as resolved
@ -38,3 +71,4 @@
content=content
)
async def send_media(self, filepath: str, filetype: Optional[str] = None, room_id: Optional[str] = None):
Owner
Copy link

Can you use the string Literal type as part of the filetype type?

Can you use the string Literal type as part of the filetype type?
charlhakeem marked this conversation as resolved
@ -41,0 +105,4 @@
if filetype in ("video", "audio"):
content["info"]["duration"] = int(float(await ffprobe(filepath, "format=duration") * 1000))
elif filetype in ("image", "video"):
Owner
Copy link

These are not exclusive, should be a second if, not elif

These are not exclusive, should be a second `if`, not `elif`
charlhakeem marked this conversation as resolved
@ -41,0 +129,4 @@
The room id of the destination of the message.
"""
if not event_id:
event_id = self.event_id
Owner
Copy link

There is no event_id attribute on Room. event_id should be required

There is no event_id attribute on Room. `event_id` should be required
charlhakeem marked this conversation as resolved
@ -41,0 +157,4 @@
uri : str
The geo URI scheme of the location.
description : str, optional
Owner
Copy link

Rename to location_description

Rename to `location_description`
charlhakeem marked this conversation as resolved
@ -41,0 +203,4 @@
question : str
The content of the question to be sent.
answers : list
Owner
Copy link

Should this be Iterable[str]? Same in the signature

Should this be `Iterable[str]`? Same in the signature
charlhakeem marked this conversation as resolved
@ -41,0 +251,4 @@
The room id of the destination of the poll.
"""
if not event_id:
event_id = self.event_id
Owner
Copy link

event_id should be mandatory

`event_id` should be mandatory
charlhakeem marked this conversation as resolved
@ -41,0 +284,4 @@
room_id = self.room_id
if not user_id:
user_id = self.user_id
Owner
Copy link

user_id should be required

`user_id` should be required
charlhakeem marked this conversation as resolved
@ -41,0 +304,4 @@
room_id = self.room_id
if not user_id:
user_id = self.user_id
Owner
Copy link

user_id should be required

`user_id` should be required
charlhakeem marked this conversation as resolved
@ -41,0 +327,4 @@
room_id = self.room_id
if not user_id:
user_id = self.user_id
Owner
Copy link

user_id should be required

`user_id` should be required
charlhakeem marked this conversation as resolved
@ -41,0 +347,4 @@
room_id = self.room_id
if not user_id:
user_id = self.user_id
Owner
Copy link

user_id should be required

`user_id` should be required
charlhakeem marked this conversation as resolved
@ -41,0 +370,4 @@
room_id = self.room_id
if not event_id:
event_id = self.event_id
Owner
Copy link

event_id should be required

`event_id` should be required
charlhakeem marked this conversation as resolved
@ -41,0 +396,4 @@
room_id = self.room_id
if not event_id:
event_id = self.event_id
Owner
Copy link

event_id should be required

`event_id` should be required
charlhakeem marked this conversation as resolved
@ -41,0 +414,4 @@
if format_as_markdown:
md = markdown.markdown(message_body,
extensions=["sane_lists", "fenced_code", "nl2br"])
body = strip_html(md)
Owner
Copy link

Remove strip_html

Remove strip_html
charlhakeem marked this conversation as resolved
@ -41,0 +445,4 @@
room_id = self.room_id
if not user_id:
user_id = self.user_id
Owner
Copy link

user_id should be required

`user_id` should be required
charlhakeem marked this conversation as resolved
@ -41,0 +454,4 @@
await self.client.room_put_state(room_id, "m.room.power_levels", content)
async def get_room_state(self, room_id: Optional[str] = None):
Owner
Copy link

Remove, out of scope

Remove, out of scope
charlhakeem marked this conversation as resolved
@ -41,0 +490,4 @@
await self.client.room_leave(room_id)
if forget:
await self.client.room_forget(room_id)
Owner
Copy link

Return Tuple of something like (Leave response/error, Optional[forget response/error])

Return Tuple of something like (Leave response/error, Optional[forget response/error])
charlhakeem marked this conversation as resolved
@ -41,2 +492,4 @@
if forget:
await self.client.room_forget(room_id)
async def join(self):
Owner
Copy link

Move this to the Bot class and add a room_id parameter

Move this to the Bot class and add a `room_id` parameter
charlhakeem marked this conversation as resolved
This pull request can be merged automatically.
You are not authorized to merge this pull request.
View command line instructions

Checkout

From your project repository, check out a new branch and test the changes.
git fetch -u v3:charlhakeem-v3
git switch charlhakeem-v3

Merge

Merge the changes and update on Forgejo.

Warning: The "Autodetect manual merge" setting is not enabled for this repository, you will have to mark this pull request as manually merged afterwards.

git switch v3
git merge --no-ff charlhakeem-v3
git switch charlhakeem-v3
git rebase v3
git switch v3
git merge --ff-only charlhakeem-v3
git switch charlhakeem-v3
git rebase v3
git switch v3
git merge --no-ff charlhakeem-v3
git switch v3
git merge --squash charlhakeem-v3
git switch v3
git merge --ff-only charlhakeem-v3
git switch v3
git merge charlhakeem-v3
git push origin v3
Sign in to join this conversation.
No reviewers
Milestone
Clear milestone
No items
No milestone
Projects
Clear projects
No items
No project
Assignees
Clear assignees
No assignees
2 participants
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference
imbev/simplematrixbotlib!91
Reference in a new issue
imbev/simplematrixbotlib
No description provided.
Delete branch "charlhakeem/simplematrixbotlib:v3"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?