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(Client): add ClientEmojiManager #4983

Closed
wants to merge 4 commits into from
Closed

feat(Client): add ClientEmojiManager #4983

wants to merge 4 commits into from

Conversation

TheNoob27
Copy link

@TheNoob27 TheNoob27 commented Nov 7, 2020

Please describe the changes this PR makes and why it should be merged:

Hey, my first Pull Request!
Having the Client#emojis getter which would cycle through every emoji, add it to a collection and all that, just to check for one emoji isn't really ideal. This ClientEmojiManager takes a different approach, and instead iterates through each guild and returns an emoji if the guild has it.
I've tested this a few times versus the previous getter, and it seems to be faster most of the time. It's definitely faster if say you want to get the first cached emoji in the bots first cached guild, because it only iterates through one emoji.
Previously, when I was using the old getter, I would often be getting memory leaks because my bot was very reaction focused and the MessageReaction#emoji getter loads every single emoji into a collection every time the getter is called, but after switching to this it seemed to all go away.

I do have to clarify that I have not been able to update the typings.

Status

  • Code changes have been tested against the Discord API, or there are no code changes
  • I know how to update typings and have done so, or typings don't need updating

Semantic versioning classification:

  • This PR changes the library's interface (methods or parameters added)
    • This PR includes breaking changes (methods removed or renamed, parameters moved or removed)
  • This PR only includes non-code changes, like changes to documentation, README, etc.

@NotSugden
Copy link
Contributor

NotSugden commented Nov 7, 2020

#4934 is taking a better approach to the issue with the class not being well suited

but the idea of this PR is there, perhaps just needs a small refactor to be more consistent with the other managers

@TheNoob27
Copy link
Author

I guess it is kinda out of place, but I decided to make it anyway because

  • it's faster, most of the time
  • it's more memory (and possibly cpu) friendly

but yeah i'll just leave this open in case anyone ever wanted to come back to this

@almostSouji
Copy link
Member

#4934 has been merged. If you have improvements to make to the other approach please open a separate PR building from the latest default branch commit.

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.

3 participants