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.

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

Utilise `tasks.loop` and webhooks for reddit postings. #501

Open
wants to merge 1 commit into
base: master
from

Conversation

@mathsman5133
Copy link
Contributor

commented Oct 8, 2019

  • The auto-posting has been removed as it was spammy and few used it.
  • Instead a daily top posting loop has been created. I stuck with top 5 daily for consistency with top 5 weekly and 5 pinned messages, but this can be easily changed
  • Both loops will post at midnight
  • As before, the weekly top posts will be pinned.
  • A webhook id for the Reddit channel has been set in constants and config, but will need to be created by an admin (and real ID set).
  • I set the webhook author to be what was previously the content of the message - it looks clean to me, but here is an example:

image

Not sure what to do about the icon, maybe a generic reddit logo would be fine (assuming copyright stuff is fine, but that might be easier set when creating the webhook?)

I've left a few notes about how to simplify the loops (by setting time parameters) when discord.py v1.3 gets released (it's in alpha now - is there any reason not to upgrade?)

Closes #391

Copy link
Member

left a comment

I think we should make use of the Tasks extension to do the work for us, instead of manually calculating and executing the delay.

bot/cogs/reddit.py Show resolved Hide resolved
bot/cogs/reddit.py Outdated Show resolved Hide resolved
@sco1 sco1 added this to Needs review in Bot Tracking Oct 9, 2019
@mathsman5133

This comment has been minimized.

Copy link
Contributor Author

commented Oct 9, 2019

I've updated the tasks to use the time parameter and combined them into one, however in upgrading discord.py to the latest (v1.3a) version it breaks the linter which seems to want to lint discord.py source, too...

@MarkKoz

This comment has been minimized.

Copy link
Member

commented Oct 14, 2019

This is why we used to have this. I guess we deemed it no longer necessary since we did not foresee having to use non-PyPI releases anymore.

And I don't know if it's OK to move to 1.3a. Hopefully someone more familiar with how things are going with that version's development can comment.

@mathsman5133

This comment has been minimized.

Copy link
Contributor Author

commented Oct 14, 2019

Sure. FWIW, I've had a (400 guild) bot running on latest versions of v1.3a for months now with absolutely no issues. I don't believe (and just had a quick look at commit history) that any breaking changes have been made so far. Instead, small additions (new methods or not breaking), bugfixes and doc changes have been made.

@scragly

This comment has been minimized.

Copy link
Member

commented Oct 14, 2019

Yeah, please avoid changing dependencies without first discussing it, especially when you're going to entirely change the release source.

For example, changing things to use v1.3a requires all our docker images and our CI to use git for installing, which we've purposely avoided. It would greatly impact our build times for CI/Docker Images thanks to pipenv version resolution and git installation taking significant extra time.

Slipping this into a PR would also result in our docker images not being ready to handle that and failing when merged to master, as well as the linter not ignoring the git-pulled lib files, because these changes weren't discussed and considered with the devops team, so the image files and linter settings weren't adjusted accordingly.

Please revert these dependency changes.

Copy link
Member

left a comment

Unnecessary major dependency source change that will negatively impact CI/CD.

@scragly

This comment has been minimized.

Copy link
Member

commented Oct 14, 2019

Thanks!

@scragly scragly dismissed their stale review Oct 14, 2019

addressed

@mathsman5133

This comment has been minimized.

Copy link
Contributor Author

commented Oct 14, 2019

I ran pipfile lock/update and it seemed to update other deps in the Pipfile.lock. Do I need to revert these too?

@scragly

This comment has been minimized.

Copy link
Member

commented Oct 14, 2019

If you've not actually changed any deps you can just reset the Pipfile and Pipfile.lock to avoid any edge cases, but for reference, updating within scope of the existing pins are usually less of a concern and are typically done anytime a new dep is added anyhow.

@mathsman5133

This comment has been minimized.

Copy link
Contributor Author

commented Oct 14, 2019

Perfect. Thanks for that, and sorry for the issues, taken note for future 👍

@mathsman5133

This comment has been minimized.

Copy link
Contributor Author

commented Oct 14, 2019

Should I resolve the conflicts?

@scragly

This comment has been minimized.

Copy link
Member

commented Oct 14, 2019

You can yep, it's up to you how you want to do it though, either by adding a new resolution commit or by doing a rebase to maybe take advantage of cleaning up history on this branch while also applying changes on top of current origin master without conflict.

@mathsman5133 mathsman5133 force-pushed the mathsman5133:reddit-makeover branch 3 times, most recently from 97e2e19 to 467eb74 Oct 14, 2019
@mathsman5133 mathsman5133 force-pushed the mathsman5133:reddit-makeover branch from 36e75bb to 2b25644 Oct 15, 2019
@mathsman5133

This comment has been minimized.

Copy link
Contributor Author

commented Oct 15, 2019

Well that was a trek and a half, but got there in the end. The last thing we need is an actual webhook ID (I've just chucked 123456789 in there for the moment)

@mathsman5133 mathsman5133 requested a review from SebastiaanZ Oct 17, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Bot Tracking
  
Needs review
4 participants
You can’t perform that action at this time.