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

feat!: Discord Library Upgrade #162

Draft
wants to merge 13 commits into
base: main
Choose a base branch
from

Conversation

dlsnyder8
Copy link
Contributor

@dlsnyder8 dlsnyder8 commented Jan 22, 2024

Summary
Upgrades the bot to use Discord.py 2.3.2

@chamburr Can you make this merge into a dev branch? I'd like to have this merged once the migration is stable, but hold off on merging into main until buttons and views are added.

Notes

  • I made a big effort w/ state.py to overwrite the fewest methods possible to avoid future big rewrites
    • I skipped Emoji caching since we'll move those over to buttons
  • I don't have context on the other class overrides for what they were attempting to accomplish @chamburr if you could add this context, that would be great

Rough todo

  • Finish class overrides
  • Clean up comments
  • Clean up unused imports
  • issue w/ startup - bot seems to hang, most likely line 240 in main.py

After this PR, but before mainstream release.

@dlsnyder8
Copy link
Contributor Author

dlsnyder8 commented Jan 22, 2024

It's not done, nor is it in a state where it works (although it doesn't crash anymore). Don't spend too much time looking through it, but if you could answer the questions I had above, that would be cool. Opening the PR to start collaboration on the update

@chamburr
Copy link
Owner

Thanks for starting this pull request!

The class overrides are rather hacky, and I can't recall what each of them accomplishes given that it's been a few years since I wrote them. It just happened that everything was needed to have the bot working at that time. I would strongly suggest to experiment and see what works since that's what I did as well.

Also, perhaps we could restrict the scope of this pull request to just the library upgrade, and leave out features like slash commands and buttons for now. Hopefully that can speed up the development cycle, which is important because this pull request would be the foundation for everything else.

@dlsnyder8
Copy link
Contributor Author

Okay we love hacky. I also wasn't sure on feasibility, but if the redis caching happened at the twilight stage, would that make this more maintainable? Since you have complete control over that - just a thought.

Agree on the scope for this PR - I meant to add spacing between the tasks to show what I wanted to do now versus before a 4.0 release. That was the reason for merging this into a dev branch rather than main.

@chamburr chamburr force-pushed the main branch 2 times, most recently from 0e5a511 to 529ada7 Compare December 10, 2024 05:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants