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

Add support for Python 3.10 by dropping event loop handling #180

Open
wants to merge 2 commits into
base: develop
Choose a base branch
from

Conversation

felixonmars
Copy link
Contributor

Implements suggestions in
#162 (comment) to
remove Pydle's internal handling of the event loop object entirely.

All tests are passing (although they are passing without these changes, too). I have been running my own project with this change and so far so good.

I am not sure about the connection pool though (I have never used it). This change should have also stopped it from having multiple event loops at all. So I have updated the stop() method from stopping the whole event loop to just asyncio.run the disconnect() method.

@Rixxan
Copy link
Contributor

Rixxan commented Aug 13, 2022

This is an issue I've also been working on, here's my implementation of the same issue fix: dc3c7ae

It looks like we went about different ways of doing the same result.

It's worth considering keeping the eventloop around and simply adding a deprecation warning for now I think, and instead of Pydle running and getting its own eventloop simply relying on the fact that Pydle will always be called from an existing asyncio loop.

This also allows hijacking of a number of existing code fascets, requiring a less intrusive method of establishing 3.10 support.

Just food for thought, I don't know if either one is "right" or "better", just another way of going about things.

Copy link
Collaborator

@theunkn0wn1 theunkn0wn1 left a comment

Choose a reason for hiding this comment

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

Hey, thanks for the PR!

I am not convinced that dropping 3.6 support is a good idea for the time being, as it is still a very common python release.
Otherwise, I very much like this approach!

@Rixxan Brings up a good point, whether we should go through a deprecation cycle first, or simply do a major release as this is PR is a breaking API change.

pydle/connection.py Outdated Show resolved Hide resolved
pydle/utils/run.py Show resolved Hide resolved
Copy link
Collaborator

@theunkn0wn1 theunkn0wn1 left a comment

Choose a reason for hiding this comment

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

Thanks for the update, Looks like something went wrong resolving the conflict in the readme.
once that is fixed I will merge this.

@Rixxan Thanks for the softer deprecation suggestion, however at this point I feel it would be better to accept this PR instead. Consequently, this PR will necessitate a major release as per SEMVER since it breaks a piece of the public API.

docs/intro.rst Outdated Show resolved Hide resolved
@theunkn0wn1 theunkn0wn1 added this to the v2.0 milestone Oct 24, 2022
Implements suggestions in
shizmob#162 (comment) to
remove Pydle's internal handling of the event loop object entirely.
Copy link
Contributor

@Rixxan Rixxan left a comment

Choose a reason for hiding this comment

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

The instructions updated in the readme appear incorrect or unsound.

Will continue to test if this is a proper readme instruction and improper edit to the connection logic or vice-versa.

@@ -65,9 +65,7 @@ Furthermore, since pydle is simply `asyncio`-based, you can run the client in yo
import asyncio

client = MyOwnBot('MyBot')
loop = asyncio.get_event_loop()
Copy link
Contributor

@Rixxan Rixxan Nov 3, 2022

Choose a reason for hiding this comment

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

This appears to be incorrect. Changing the entry logic for my own implementation of pydle results in a hung program, which immediately terminates uncleanly.

However, testing on Python 3.8 if I do not change my existing entry logic, the program starts properly. (Still stability-testing.)

The same behavior replicates on 3.10.

bmwiedemann pushed a commit to bmwiedemann/openSUSE that referenced this pull request Feb 24, 2023
https://build.opensuse.org/request/show/1067570
by user dgarcia + dimstar_suse
- Delete python-pydle-poetry-syntax.patch
- Skip python 310 and python 311, pydle doesn't python 3.10
  gh#shizmob/pydle#162, There's a WIP PR but looks like it's not good
  enough yet to be applied gh#shizmob/pydle#180
- Update to version 1.0.1
  * [Docs] Document Dropping of 3.5 by @Rixxan in #179
  * Adjust python version inequality string for Poetry by @txtsd in #182
  * bump version to 1.0.1 by @theunkn0wn1 in #183
@TheHolyRoger
Copy link

I haven't tested this yet as I'm just starting a brand new project but I think this might be the "more correct" way to handle the asyncio changes in py3.10+

asyncio.open_connection as per the documentation is a high level function call and should ideally not be used in libraries that allow full manipulation of event loops, which is why the loop argument was dropped.

If there's no event loop it will create a new event loop and set it as the main one

TheHolyRoger@befd10f

@TheHolyRoger
Copy link

@Rixxan you might wanna have a look :)

@Rixxan
Copy link
Contributor

Rixxan commented May 20, 2024

TheHolyRoger's branch fix also seems to work cleanly.

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.

5 participants