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.

Sign up for GitHub

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

Jump to bottom

refactor: split _dispatch_event in two methods #1008

Merged
merged 40 commits into from Nov 26, 2022
Merged

refactor: split _dispatch_event in two methods #1008

merged 40 commits into from Nov 26, 2022

Conversation

Damego
Copy link
Member

@Damego Damego commented Aug 7, 2022 •
edited

About

This pull request splits _dispatch_event in two other methods.

Checklist

  • I've ran pre-commit to format and lint the change(s) made.
  • I've checked to make sure the change(s) work on 3.8.6 and higher.
  • This fixes/solves an Issue (If existent):.
    • resolves #
  • I've made this pull request for/as: (check all that apply)
    • Documentation
    • Breaking change
    • New feature/enhancement
    • Bugfix

@Damego
Copy link
Member Author

Damego commented Aug 7, 2022

Can I include #1004 into this pr?

@EdVraz EdVraz added the on hold This issue/PR is on hold pending higher priority issues/PRs label Aug 9, 2022
@Damego
Copy link
Member Author

Damego commented Aug 10, 2022

Resolved merge conflict lol. Why it shows changes which already in unstable?

@Damego
Copy link
Member Author

Damego commented Aug 20, 2022

oh damn... rebase in third time

@EdVraz
Copy link
Contributor

EdVraz commented Aug 20, 2022

oh damn... rebase in third time

Wait until #1022

@Damego
Copy link
Member Author

Damego commented Aug 20, 2022

okay

@Damego
Copy link
Member Author

Damego commented Aug 20, 2022

Will mark as draft

@Damego Damego marked this pull request as draft August 20, 2022 08:25
@Damego Damego marked this pull request as ready for review August 21, 2022 16:12
@Damego
Copy link
Member Author

Damego commented Sep 4, 2022

I hate this pr

@EdVraz
Copy link
Contributor

EdVraz commented Sep 4, 2022

I hate this pr

Perfect

@Damego Damego marked this pull request as draft September 4, 2022 08:08
@Damego Damego changed the base branch from unstable to stable September 4, 2022 08:12
@Damego Damego changed the base branch from stable to unstable September 4, 2022 08:13
@Damego Damego marked this pull request as ready for review September 4, 2022 08:16
@Damego
Copy link
Member Author

Damego commented Sep 4, 2022

Its fine now

@Damego
Copy link
Member Author

Damego commented Oct 5, 2022

I will reopen this fucking pr

@Damego Damego closed this Oct 5, 2022
@Damego Damego reopened this Oct 5, 2022
@Damego Damego marked this pull request as draft October 5, 2022 08:17
@Damego
Copy link
Member Author

Damego commented Oct 5, 2022

fine

@Damego Damego marked this pull request as ready for review October 5, 2022 08:31
@Damego Damego changed the title refactor: split dispatch in two methods refactor: split _dispatch_event in two methods Oct 5, 2022
@Catalyst4222 Catalyst4222 removed the on hold This issue/PR is on hold pending higher priority issues/PRs label Oct 29, 2022
@Catalyst4222
Copy link
Contributor

Can you resolve conflicts and re-test?

@Damego Damego changed the base branch from unstable to stable October 30, 2022 05:06
@Damego Damego changed the base branch from stable to unstable October 30, 2022 05:06
@Damego
Copy link
Member Author

Damego commented Nov 3, 2022

image
Merge it pls. Every new rebase takes more time than previous

@EdVraz
Copy link
Contributor

EdVraz commented Nov 22, 2022

image Merge it pls. Every new rebase takes more time than previous

I find it satisfying for how long this PR is already opened without being merged and how it just gets worse and worse xD

EdVraz
EdVraz reviewed Nov 22, 2022
View reviewed changes
Copy link
Contributor

@EdVraz EdVraz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Has this been tested with EVERY event and type of command as well as every type of select, different amounts of select menus, etc etc?

@Damego
Copy link
Member Author

Damego commented Nov 23, 2022 •
edited

This pr does not changes code or logic of dispatching. Im just moved code in function. I know its impossible to review this pr and better just look at my branch

@Damego
Copy link
Member Author

Damego commented Nov 23, 2022

image Merge it pls. Every new rebase takes more time than previous

I find it satisfying for how long this PR is already opened without being merged and how it just gets worse and worse xD

This pr contain absolutely same code we have on unstable branch soooo do you mean our code gets worse and worse then lol?

@EdVraz
Copy link
Contributor

EdVraz commented Nov 23, 2022

image Merge it pls. Every new rebase takes more time than previous

I find it satisfying for how long this PR is already opened without being merged and how it just gets worse and worse xD

This pr contain absolutely same code we have on unstable branch soooo do you mean our code gets worse and worse then lol?

Fine but has it been tested enough to ensure that you did everything correctly? So we know that you did not indent something wrong etc

@Damego
Copy link
Member Author

Damego commented Nov 23, 2022

I tested it right now and yes, it works fineee (as expected).
_dispatch_event properly calls methods to dispatch interactions and discord events.

@Damego
Copy link
Member Author

Damego commented Nov 23, 2022

Review it here https://github.com/Damego/library/blob/ref-split-dispatch/interactions/api/gateway/client.py#L362

@EdVraz
Copy link
Contributor

EdVraz commented Nov 23, 2022

looks like it needs another rebase (sorry)

@Damego Damego requested a review from EdVraz November 24, 2022 05:03
@Damego Damego merged commit 5e41552 into interactions-py:unstable Nov 26, 2022
@Damego Damego deleted the ref-split-dispatch branch December 6, 2022 13:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@EdVraz EdVraz EdVraz approved these changes

@Catalyst4222 Catalyst4222 Awaiting requested review from Catalyst4222

@AstreaTSS AstreaTSS Awaiting requested review from AstreaTSS

@FayeDel FayeDel Awaiting requested review from FayeDel

@Toricane Toricane Awaiting requested review from Toricane

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@Damego @EdVraz @Catalyst4222