Skip to content
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.

Sign up for GitHub

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

Jump to bottom

feat: handle long tweet #1339

Merged
merged 8 commits into from
Dec 27, 2024
Merged

feat: handle long tweet #1339

merged 8 commits into from
Dec 27, 2024

Conversation

tcm390
Copy link
Collaborator

@tcm390 tcm390 commented Dec 21, 2024 •
edited
Loading

related:
#1178

This PR handles long tweets as follows:

  1. Check if the post is a long post. If it’s not, proceed with a normal tweet. Otherwise, use the sendNoteTweet method for long tweets.
  2. If sendNoteTweet fails, truncate the post and fall back to using a normal tweet.

test result:

  1. non-verfied user post: https://x.com/IBGDRGNAI/status/1870505476284469692
  2. verified user post: https://x.com/JayChou11879/status/1870505908834615527

odilitime
odilitime reviewed Dec 21, 2024
View reviewed changes
Copy link
Collaborator

@odilitime odilitime left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code LGTM, important fix in there too

@odilitime odilitime changed the title handle long tweet feat: handle long tweet Dec 21, 2024
@ChristopherTrimboli ChristopherTrimboli mentioned this pull request Dec 21, 2024
Closed
@odilitime
Copy link
Collaborator

odilitime commented Dec 21, 2024 •
edited
Loading

duplicate of #1335 draft

@odilitime
Copy link
Collaborator

Duplicate of #1329 too?

@tcm390
Copy link
Collaborator Author

tcm390 commented Dec 21, 2024 •
edited
Loading

Duplicate of #1329 too?

Yes @ChristopherTrimboli already had a pr to fix double tweet issue. We should open his pr and merge his pr first: #1329

@ChristopherTrimboli
Copy link
Contributor

Duplicate of #1329 too?

Yes @ChristopherTrimboli already had a pr to fix double tweet issue. We should open his pr and merge his pr first

im fine, I discussed with TCM, honest mistake, in open-source some saltiness if devs get PRs yoinked, but was legit coincidence, idc that much, just a 1 liner. I feel welcome after TCM chat + role upgrade, not worried.

Base automatically changed from develop to main December 22, 2024 07:01
@shakkernerd shakkernerd changed the base branch from main to develop December 22, 2024 08:23
@shakkernerd shakkernerd mentioned this pull request Dec 27, 2024
Closed
@tcm390
Copy link
Collaborator Author

tcm390 commented Dec 27, 2024

resolved conflict and tested

verified account: https://x.com/JayChou11879/status/1872688847106044244
non-verified-account: https://x.com/IBGDRGNAI/status/1872689915114475839

@shakkernerd shakkernerd merged commit 1c78910 into develop Dec 27, 2024
7 checks passed
@shakkernerd shakkernerd deleted the tcm-long-tweet branch December 27, 2024 17:57
This was referenced Dec 27, 2024
Closed
Closed
@odilitime odilitime mentioned this pull request Dec 28, 2024
Merged
@oxSaturn oxSaturn mentioned this pull request Dec 28, 2024
Merged
@tcm390 tcm390 mentioned this pull request Jan 7, 2025
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@odilitime odilitime odilitime left review comments

@shakkernerd shakkernerd shakkernerd approved these changes

Assignees
No one assigned
Labels
Needs Testing
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@tcm390 @odilitime @ChristopherTrimboli @shakkernerd