-
Notifications
You must be signed in to change notification settings - Fork 7
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
Fix twitter not embedding in the first place (fxtwitter). Add support for tiktok (vxtiktok), reddit (fxreddit/rxddit), and threads (vxthreads) #12
base: dpy2
Are you sure you want to change the base?
Conversation
Formatted with Black
Add fxtwitter/fixupx link conversion
Also closes #13 |
68e754e adds support for https://github.com/MinnDevelopment/fxreddit and https://github.com/everettsouthwick/vxThreads It also fixes some regex, the "."s in URLs weren't escaped. So, i.e., "instagramZcom" would have triggered the bot |
Sometimes it fails to notice/fix a link. Reposting usually works. Not sure if thats lag on my test bot or what. At any rate, the code is a bit of a mess with 3 more servivces, might need restructuring. |
CORRECTION: It doesn't work if it takes a while for discord to add the original embed (i.e. for it to load the link + gen the embed) Not sure how to fix that, beyond switching all the logic from looking for embeds, to just any url whether its embeded or not (like I've had to do for twitter) |
Since payload.data["content"] is optional
Yes. This PR should be split into smaller ones. I imagine something like this:
One PR can be created each of the above. Preferably there's an issue attached to each "add support for something" kind of PR because then you can state why it is necessary and have a separate room for discussions there, but as I see it, if you are willing to implement and test all of this anyway, then some SNS additions to the cog don't sound that bad. Having small PRs as I described will make it easier to:
In PR posts, let us know what areas of code you want extra review, like you're already doing now. Make sure that if you're doing manual testing (which is likely what you'll have to do for this cog for now anyway), then be sure to include images/videos of the tests that you've done. Also, note anything that is specific to a(n) SNS when you do it. If something in the existing code should be made configurable (e.g., the follow-up messages that the bot posts after detecting a fixable link), do bring it up for discussions as well. We will also need to adjust the casing of names in this cog's code because we want |
Yeah ngl I wanted to split them but I only use github for personal projects (made this fork anyway to use for my own server) so didn't know a good way to split all the commits for different pulls after the fact; plus was worried about merges when I've made changes to the base code which they all rely on. I've tested each service, they're all successfully replacing the links now. If you do still want seperate PRs, am I gonna need to refork, make new branches for each, then port over the code? Or is there a better way. I'll have a look when I get home. |
What I meant by "code needs restructuring" is that, with 5 different services, there's a lot of repetition now. I reckon at this rate a single handler applying multiple regex search/replaces would be more efficient. Perhaps split into a text content replacer (twitter) and an embed replacer (everything else) |
I agree that we can do some refactoring to reduce code repetition, so I'd say if you're down to make the change, feel free to do a PR after adding support for these individual SNSes.
I suggest that in whatever the fork that you wanna use to do PRs, for each action item (that eventually will become a PR), make a branch. Let's say for the change where you wanna refactor the existing code, do a branch that branches from origin's To flexibly work with local changes and branches, you need to learn commit rebasing. I think you only need to know basic rebasing (e.g., rebasing branch B on branch A after making additional commits in branch A, assuming B depends on A), but if you can organize your work well in advance, you won't even have to do that. Also, because in this PR, you can see all the changes you wanted to do, you should have an idea of what code you should put in a smaller PR anyway. If you work on a branch that is about refactoring the existing code, then just take the specific portion of code from this PR to the branch in your Git workspace, then push it and form a PR with it. After that, move on to the ones that add support for various SNSes. Code review will be resumed when the splitting task is done. |
Closes #11
(My new twitter code sucks ass, please review it. doing a str.split(), then running regex on each string. and excluding anything with an @ just incase some
@everyone
method exists. i dont code for discord lol)