pycord icon indicating copy to clipboard operation
pycord copied to clipboard

feat: event redesign

Open EmmmaTech opened this issue 2 years ago • 5 comments

Summary

This redesigns the event system in discord.Client according to #189.

Checklist

  • [x] If code changes were made then they have been tested.
    • [ ] I have updated the documentation to reflect the changes.
  • [ ] If type: ignore comments were used, a comment is also left explaining why
  • [ ] This PR fixes an issue.
  • [x] This PR adds something new (e.g. new method or parameters).
  • [ ] This PR is a breaking change (e.g. methods or parameters removed/renamed)
  • [ ] This PR is not a code change (e.g. documentation, README, typehinting, examples, ...)

EmmmaTech avatar May 06 '22 01:05 EmmmaTech

I don't really see the enum used anywhere except for passing to an event decorator, so not sure why that's needed

Make sure the Client subclasses support this as well, and custom events should still work

Changing the rest of the code to use the enum is a WIP, hence why this PR was opened as a draft.

EmmmaTech avatar May 06 '22 02:05 EmmmaTech

I don't really see the enum used anywhere except for passing to an event decorator, so not sure why that's needed

Make sure the Client subclasses support this as well, and custom events should still work

Changing the rest of the code to use the enum is a WIP, hence why this PR was opened as a draft.

Didn't notice it was a draft, apologies

plun1331 avatar May 06 '22 02:05 plun1331

I don’t see why this is needed? Seems useless from my perspective. also replace "according to #189." with "closes #189"

Do you have constructive feedback to provide? Saying "seems useless" is not helpful unless you're able to provide more specifics.

Personally I think this PR is a good idea and will help newcomers to the library dispatch events easier.

krittick avatar May 16 '22 23:05 krittick

I don’t see why this is needed? Seems useless from my perspective. also replace "according to #189." with "closes #189"

Do you have constructive feedback to provide? Saying "seems useless" is not helpful unless you're able to provide more specifics.

Personally I think this PR is a good idea and will help newcomers to the library dispatch events easier.

It only adds an enum, nothing else is changed, it doesn’t even optimize the current systems slowdowns. I honestly think a class-based event system like hikaris would be better, if the current system needs changing at all.

The enum values for events also seem to be undocumented, I find it better for newcomers if these were instead documented, like the current events.

VincentRPS avatar May 17 '22 02:05 VincentRPS

I don’t see why this is needed? Seems useless from my perspective. also replace "according to #189." with "closes #189"

Do you have constructive feedback to provide? Saying "seems useless" is not helpful unless you're able to provide more specifics.

Personally I think this PR is a good idea and will help newcomers to the library dispatch events easier.

It only adds an enum, nothing else is changed, it doesn’t even optimize the current systems slowdowns. I honestly think a class-based event system like hikaris would be better, if the current system needs changing at all.

The enum values for events also seem to be undocumented, I find it better for newcomers if these were instead documented, like the current events.

First of all, about the documentation thing: this PR is still a work in progress. So stuff like documentation haven't been added yet because this isn't entirely finished yet.

Second of all, this does change one thing about the event system according to the original feature request: instead of setting all of the event handlers as literal attributes of the Client, it's just in a dictionary. Sure, it's not a major change, but it is a change for sure.

EmmmaTech avatar May 17 '22 05:05 EmmmaTech