-
Notifications
You must be signed in to change notification settings - Fork 221
[chore] Automate creation of release notes toc #1155
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
|
Auto-sync is disabled for ready for review pull requests in this repository. Workflows must be run manually. Contributors can view more details about this message here. |
|
/ok to test |
This comment has been minimized.
This comment has been minimized.
|
Yeah, this works fine in the individual builds, but isn't playing nice with the combined build, it looks like. I'll see what we can do... |
|
/ok to test |
|
@rwgk: I think this addresses everything you raised now. |
|
/ok to test |
Oh, I see that the links across the projects in the preview take you out to the published docs. In that case, I think there's still something wrong here. Not sure why, since it's working locally for me. I will investigate. |
|
/ok to test |
@rwgk: This is checking out now. I'd appreciate a second look. Thanks. |
rwgk
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.
Looks good to me. I looked through the generated pages pretty carefully, it looks perfect.
My suggestions are optional; maybe for another PR after the release is out.
| super().parse_content(toctree) | ||
|
|
||
| if not toctree["glob"]: | ||
| return |
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.
Do we have a valid use case that needs this? — If not: maybe issue a warning, to guard against oversights, like typos?
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.
The main toctree in each of the subprojects doesn't use glob, they list the files explicitly. We don't want to sort those. For example.
cuda_python/docs/exts/release_toc.py
Outdated
| toctree["entries"] = [ | ||
| (Version(Path(x[1]).name.removesuffix("-notes")), x[1]) for x in toctree.get("entries", []) | ||
| ] | ||
| toctree["entries"].sort(key=lambda x: x[0], reverse=True) | ||
| toctree["entries"] = [(str(x[0]), x[1]) for x in toctree["entries"]] |
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.
WDYT about the below?
- to make the code more hermetic (doesn't assign multiple times to
toctree["entries"]) - issues a warning to guard against oversights
I didn't test this:
entires_in = toctree.get("entries")
if not entires_in:
warnings.warn('toctree["entries"] is EMPTY') UserWarning, stacklevel=2)
return
entries = [(Version(Path(x[1]).name.removesuffix("-notes")), x[1]) for x in entires_in]
entries.sort(key=lambda x: x[0], reverse=True)
toctree["entries"] = [(str(x[0]), x[1]) for x in entries]
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 makes sense. Sphinx itself already warns when glob finds no elements, so we don't need to do that again.
|
/ok to test |
|
LGTM, need to resolve the conflicts |
|
Discussed with Mike offline, we could move the plugin to a common place, e.g. |
|
/ok to merge |
|
/ok to test |
@mdboom, there was an error processing your request: See the following link for more information: https://docs.gha-runners.nvidia.com/cpr/e/1/ |
|
/ok to test 475fa5c |
|
This prevents the need to update the release notes table of contents every time we make a release. It automatically pulls in all release notes files (ignoring ones that don't end in a digit, since we don't want to include pre-release release notes). A sphinx extension is required to sort them in the desired order, which is "newest first, by version number".