-
Notifications
You must be signed in to change notification settings - Fork 291
feat!(timeline): allow sending media as (thread) replies #4852
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
5bfb877
to
38cca2f
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4852 +/- ##
=======================================
Coverage 86.55% 86.56%
=======================================
Files 300 300
Lines 34966 34980 +14
=======================================
+ Hits 30266 30281 +15
+ Misses 4700 4699 -1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Signed-off-by: Johannes Marbach <[email protected]>
38cca2f
to
acc8ce3
Compare
/// Optional parameters for sending the media as (threaded) reply. | ||
reply_params: Option<ReplyParameters>, |
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've added this here to not have to duplicate the send_*
methods for the various media types. Maybe we could also use this to merge send_reply
and send_thread_reply
?
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.
Yep, that would make sense (maybe as a followup PR, to not have this one drag for too long?)
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.
Separate PR makes sense. 👍
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.
Thanks! A first round of review comments here, but it looks like this is on the right track!
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.
Nice, we're getting close to landing this! LGTM on principle that we can address the last comments without large changes.
/// Optional parameters for sending the media as (threaded) reply. | ||
reply_params: Option<ReplyParameters>, |
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.
Yep, that would make sense (maybe as a followup PR, to not have this one drag for too long?)
Thanks a lot for your patience with me figuring things out through multiple rounds of review here. 🙏 |
No worries, thanks a lot for this contribution! |
This implements step 3 of #4835.