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
Update maintenance goals #850
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. |
@@ -47,34 +63,19 @@ To build a complete set of HTML documentation, you must have Sphinx, which can b | |||
|
|||
The built html files can be found in doc/_build/html afterward. | |||
|
|||
|
|||
Twisted |
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.
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.
Supported Python versions | ||
========================= | ||
|
||
Python 3.7-3.12 are currently supported. | ||
|
||
Flair |
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
[1] https://sublime-and-sphinx-guide.readthedocs.io/en/latest/notes_warnings.html
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 |
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good points, let me drop that sentence.
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same thing, I would delete this paragraph for now.
@@ -1,3 +1,55 @@ | |||
Warning |
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.
GitHub actually has syntax for this: https://github.com/orgs/community/discussions/16925
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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. |
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"Eventlet has now been switched to legacy mode"?
@@ -12,7 +63,7 @@ applications to use it. Start off by looking at the `examples`_, | |||
|
|||
|
|||
Quick Example | |||
=============== | |||
============= |
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.
IMO this example only serves to attract new uses, so I'd suggest to drop it completely, too.
See the commit message for more details.
Fix #835