-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
Generate a release schedule as an .ics file
#4705
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
base: main
Are you sure you want to change the base?
Conversation
Co-authored-by: Hugo van Kemenade <1324225+hugovk@users.noreply.github.com>
| global _PYTHON_RELEASES | ||
| if _PYTHON_RELEASES is not None: | ||
| return _PYTHON_RELEASES |
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 think chucking a @cache decorator on the function (from the original PR) is much cleaner than six lines to manage a global variable.
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.
@functools.cache is shorter but also overkill IMO, we don't need the LRU complexity etc.
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.
It's not complex, it's a black box decorator :)
We don't need to worry what it's doing under the hood, the complexity that matters is the code we're looking at here.
And functools.cache is 1.4x as fast as the global.
Or if it's a problem, let's just ditch this caching altogether. It's only called three times, so it's not like it'll make any noticeable difference.
| global _PYTHON_RELEASES | ||
| if _PYTHON_RELEASES is not None: | ||
| return _PYTHON_RELEASES |
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.
It's not complex, it's a black box decorator :)
We don't need to worry what it's doing under the hood, the complexity that matters is the code we're looking at here.
And functools.cache is 1.4x as fast as the global.
Or if it's a problem, let's just ditch this caching altogether. It's only called three times, so it's not like it'll make any noticeable difference.
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.
Let's also skip measuring coverage for if __name__ == '__main__': blocks, it's not worth testing.
I also suggested skipping https://github.com/python/peps/blob/main/release_management/__main__.py in the original PR. Is there anything special worth testing in there?
| 'X-WR-CALNAME:Python releases schedule', | ||
| ] | ||
| for pep_number, release in releases: | ||
| normalised_stage = release.stage.casefold().replace(' ', '') |
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.
Why casefold?
These are all lowercase, trusted strings with predictable contents like "3.14.0 alpha 1", "3.14.0 candidate 3" and "3.14.5".
They won't have any uppercase or German "ß".
| normalised_stage = release.stage.casefold().replace(' ', '') | |
| normalised_stage = release.stage.replace(' ', '') |
This is derived from Hugo's #4704; see that PR for context.
This PR is an attempt to implement @sethmlarson's suggestion -- attempting to do this via review comments wasn't really going to work!
The ICS generation is now much simpler with no dependencies, simply outputting the required strings. I've also limited past events to 7 years to keep the file size smaller (for ~18months of pre-releases and a five-year lifespan). Realistically this could be much shorter, but I've been conservative.
A
📚 Documentation preview 📚: https://pep-previews--4705.org.readthedocs.build/