search

Injabie3 / lui-cogs-v3

Cogs for Red-DiscordBot (v3), used by the SFU Anime Club. Branches for dpy1 and dpy2. See Injabie3/lui-cogs for Redv2/dpy0 cogs.
14 stars 7 forks source link

Fix twitter not embedding in the first place (fxtwitter). Add support for tiktok (vxtiktok), reddit (fxreddit/rxddit), and threads (vxthreads) #12

Open DoctorDinosaur opened 10 months ago

DoctorDinosaur commented 10 months ago

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)

DoctorDinosaur commented 10 months ago

Also closes #13

DoctorDinosaur commented 10 months ago

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

DoctorDinosaur commented 10 months ago

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.

DoctorDinosaur commented 10 months ago

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)

DoctorDinosaur commented 10 months ago

04b07ff seems going to on_raw_message_edit and restructuring around that fixes #14 Buggy mess tho, again pls review

quachtridat commented 10 months ago

At any rate, the code is a bit of a mess with 3 more servivces, might need restructuring.

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 camelCase rather than snake_case, but that can be done later anyways.

DoctorDinosaur commented 10 months ago

Yes. This PR should be split into smaller ones.

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. And switching to raw_edit detection has fixed the issue with them not being picked up in the first place cos discord's embed server is slow to add the embeds for new/unseen links.

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.

DoctorDinosaur commented 10 months ago

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)

quachtridat commented 10 months ago

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.

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.

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.

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 dpy2. Then, when you wanna do something that requires the refactoring work you did in that branch (e.g., adding support for TikTok), you can make another branch off of that branch, then work there. After you've got the work done, you can start some PRs with those specific branches.

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.