Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #850 +/- ##
========================================
+ Coverage 53% 61% +7%
========================================
Files 88 89 +1
Lines 9869 12075 +2206
Branches 1848 2825 +977
========================================
+ Hits 5325 7449 +2124
- Misses 4156 4205 +49
- Partials 388 421 +33
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
| The built html files can be found in doc/_build/html afterward. | ||
|
|
||
|
|
||
| Twisted |
There was a problem hiding this comment.
This section was added in 2014 and the twisted support have been removed since eventlet 0.14.0. We are now close to 2024, I think we can safely remove this section. The message have been passed.
|
|
||
| Python 3.7-3.12 are currently supported. | ||
|
|
||
| Flair |
There was a problem hiding this comment.
badges are more useful at the beginning of README files
README.rst
Outdated
| .. image:: https://codecov.io/gh/eventlet/eventlet/branch/master/graph/badge.svg | ||
| :target: https://codecov.io/gh/eventlet/eventlet | ||
|
|
||
| .. warning:: |
There was a problem hiding this comment.
Looks like rst notes and warnings [1] are not recognized by github's rendering engine. Do you think we should simply bold the following sentence?
There was a problem hiding this comment.
How about a) make "Warning" a chapter header? b) move this even in front of the "Eventlet" chapter? After all, this likely is the most important information on this page for anyone reading this doc for the first time.
There was a problem hiding this comment.
Good idea. I'll update the patch to match your proposal. Will do that after my PTO.
README.rst
Outdated
|
|
||
| .. image:: https://img.shields.io/github/actions/workflow/status/eventlet/eventlet/test.yaml?branch=master | ||
| :target: https://github.com/eventlet/eventlet/actions?query=workflow%3Atest+branch%3Amaster | ||
| Eventlet was created almost 18 years ago, at a time where async features |
There was a problem hiding this comment.
This is important enough that I would either move it to the top, or add a sentence to the top saying "New usages of eventlet are now heavily discouraged, see below for details."
There was a problem hiding this comment.
I agree, I'll rework that part.
|
I assume you only want to merge this in January, per comments on the issue? |
8e3704b to
95108e2
Compare
Absolutely, however, I prefer to publicly designing this pull request starting now, to allow people to see our intentions well before merging this patch. I'll drop the WIP flag early in January. Is it ok for you? |
95108e2 to
43c7130
Compare
|
If you do another pass we can include this in next release? |
|
Yes, that's what I was thinking. I was thinking that the next release would be related to the hub and to our new maintenance goals. May, with the next release, we can provide a major version. A version 1.0. That would highlight the stable character related to our new maintenance goal (only bugfixes, and security stuffs allowed). Also that would provide a strong signal at destination to the community to socialize the new asyncio hub. @itamarst : Any opinion? |
|
I'm currently updating this pull request. I take account of your remarks and I'm now rewriting sentences to speak about the new asyncio hub. I'll push my changes today. |
43c7130 to
74c9aa9
Compare
74c9aa9 to
290fda6
Compare
done |
itamarst
left a comment
There was a problem hiding this comment.
A few minor suggestions, then merge — thank you!
README.rst
Outdated
| and if you do not yet use eventlet, then, we encourage you to use `asyncio`_, | ||
| which is the official async library of the CPython stdlib. | ||
|
|
||
| If you already use eventlet, a new asyncio hub has been recently added to |
There was a problem hiding this comment.
I would omit this paragraph for now, since:
- This hub is still very minimally tested. I would like e.g. a few OpenStack projects to run their test suites, at an absolute minimum.
- Merely using the hub doesn't help with migration, there's a bunch of additional infrastructure (in progress) and documentation needed.
- Some users can't realistically migrate this way. In particular there's projects like Celery that specifically rely on having an API that looks blocking, so "rewrite with asyncio" doesn't meet that use case. Might be a bad desire, but it's still a different case than OpenStack. So I suspect we'll want different advice for different users ("you can use gevent, although we believe it will have similar issues" for celery-like users I guess).
There was a problem hiding this comment.
Good points, let me drop that sentence.
There was a problem hiding this comment.
What do you think if we reword this sentence to simply indicate that we are thinking about a migration plan?
Maybe would should less formal about the existence of this feature, but we could simply indicate that we are looking for a solution to allow user migrating from eventlet to asyncio without much details for now, thoughts?
doc/index.rst
Outdated
| and if you do not yet use eventlet, then, we encourage you to use `asyncio`_, | ||
| which is the official async library of the CPython stdlib. | ||
|
|
||
| If you already use eventlet, a new asyncio hub has been recently added to |
There was a problem hiding this comment.
Same thing, I would delete this paragraph for now.
| @@ -1,3 +1,55 @@ | |||
| Warning | |||
There was a problem hiding this comment.
GitHub actually has syntax for this: https://github.com/orgs/community/discussions/16925
There was a problem hiding this comment.
That's not a markdown file, it is a restructredText file, and I don't think this syntaxe is supported by Pypi ...
There was a problem hiding this comment.
I'd suggest keeping a compatible solution. The current proposed one.
- Discourage new eventlet usages - Encourage usages of asyncio for network programming - Speak about our intentions to allow transitions from eventlet to asyncio - Highlight our plan to retire eventlet - Maintenance only for bugfixes and security purposes - New features are not accepted
290fda6 to
a674d89
Compare
|
@itamarst: Let me know if the latest version of this patch is ok for you, especially the reword of the asyncio hub sentence. |
osfrickler
left a comment
There was a problem hiding this comment.
I'm fine with this as is and I think it is more important to get this merged than to discuss details of formatting and wording, so please read my comments more as ideas for a possible followup.
|
|
||
| **Eventlet is now switching to legacy mode**. **Only maintenance for stability | ||
| and bug fixing** will be provided. **No new features will be accepted**, except | ||
| those related to the asyncio migration. **Usage in new projects are |
There was a problem hiding this comment.
Either "Usage in new projects is ..." or "Usages in new projects are ..."
| This gap is now too high and can lead you to unexpected side effects and bugs | ||
| in your applications. | ||
|
|
||
| **Eventlet is now switching to legacy mode**. **Only maintenance for stability |
There was a problem hiding this comment.
"Eventlet has now been switched to legacy mode"?
|
|
||
| Quick Example | ||
| =============== | ||
| ============= |
There was a problem hiding this comment.
IMO this example only serves to attract new uses, so I'd suggest to drop it completely, too.
Gunicorn should discourage new usages accordingly, see eventlet/eventlet#850
Gunicorn should discourage new usages accordingly, see eventlet/eventlet#850
See the commit message for more details.
Fix #835