-
Notifications
You must be signed in to change notification settings - Fork 39
Rework connection classes with proper async support #273
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
Conversation
|
Hi @CoolCat467, thanks for the rebase (btw, you don't need to make a new PR when rebasing, we do allow force pushing). I've edited your PR description to include some more detailed things about what this is doing (mostly based on the previous PR description), but most importantly, I've added some TODO check boxes there which needs to be done before this can be reviewed and then merged. I've quickly glanced over the changes this introduces and it looks pretty interesting, but until it meets these TODOs and is compliant with our code quality standards, it can't be merged. Because of that, I've marked your PR as draft to avoid cluterring other PRs which are actually ready to be merged already. Once you feel like the PR is ready to be reviewed and you fulfilled these points and are passing the automated workflow checks, you can mark it as Ready for review (this button): If you need more info on how to fulfill some of these, or how to pass the automated workflows, feel free to ask on discord, or here as a comment (though discord will probably be better for longer discussions). |
|
Ok, thank you for the information. |
|
@CoolCat467, do you plan on going through with this and implementing it fully or can someone take over this PR? |
|
I've never written unit tests before, so that would probably be for the best. |
Alright, no problem, I don't mind finishing this up, although it may take me a while since I've been a bit busy lately, so if someone else would want to, feel free to take over, otherwise I'll implement this hopefully within a few weeks time. |
kevinkjt2000
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are a handful of type checking errors when running tox -e lint. See CONTRIBUTING.md for the commands to set up your terminal. Once those type checks are fixed, I'd start running tox -e py310 to execute tests for a specific version. When I get a single version working, I like to use both the oldest and newest tox -e py37,py310. To run the whole suite of format check, linting, and tests: tox.
If you ping me on the Discord server, I could schedule some time to teach you some unit test stuff if you're interested. Unit tests are basically a collection of a bunch of miniature Python scripts that have assertions sprinkled into them.
I'll wait until things are passing again to do a more thorough review of the new connection classes, but I'm on board with this idea. Thanks for taking the time on this PR! My apologies for taking a while to get around to this.
Co-authored-by: Kevin Tindall <kevinkjt2000@users.noreply.github.com>
Co-authored-by: Kevin Tindall <kevinkjt2000@users.noreply.github.com>
Co-authored-by: Kevin Tindall <kevinkjt2000@users.noreply.github.com>
ItsDrike
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a quick review purely addressing the formatting/typing issues, I'll make another review on the actual logic after these changes are addressed. Thanks for the contribution though, the connection classes really needed an update for a while now!
Co-authored-by: ItsDrike <itsdrike@protonmail.com>
ItsDrike
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great job! The code does still need a lot of minor formatting changes, but logic-wise I don't see any significant issues to point out (other than some that were already there before this PR, which should be fixed elsewhere anyway).
Fix formatting issues in doc strings, default values to initialize to, a few small type annotation things. Co-authored-by: ItsDrike <itsdrike@protonmail.com>

Rebase of #225.
Closes #210
This PR introduces a complete rewrite of the protocol classes and fixes a bug, in which asynchronous connections were sometimes using synchronous functions when asynchronous ones were expected.
It adds doc strings to all of the methods, introduces some memory optimizations with the use of
__slots__in the connection classes, and separatesConnectioninto several abstract classes, both synchronous and asynchronous, which then together compose the sync and asynchronous Connection classes. These abstract classes prevent a lot of previously duplicate code.TODO:
Adjust the rest of the code base to work with this rewritten codeNo changes should be required, but improvements could be made