-
Notifications
You must be signed in to change notification settings - Fork 52
Add admonitions for new and changed APIs in 2022 version #585
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
Thanks @steff456. The changes look good mostly; one thing to add is to ensure there's a Notes section when you add the directives at the bottom. Otherwise you'll get rendering issues like this (for
I suggest to do and review it in this PR for the draft version. Once it's merged, let's sync it to the 2022 version. That should be easy, I don't think we've merged other changes to these files, so copying files should get you the right diff.
I don't see an issue. Maybe also ensure you delete the |
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 LGTM now. @steff456 I reverted the changes to the Raises sections, because the relevant docstrings all had rendering issues. It doesn't look fixable to me, because the Raises section expects the raised exception to be listed. We don't specify that, which is why the rendering was off.
This PR,
I guided myself using the changelog for both the changed and new APIs.
Some questions,
draft
version? Or do we also want them in the 2022 spec?make
and the documents are not re-rendered correctly. I'm not sure if this is only happening on my side, tomorrow I will test it in a new env and if the error persists I'll open an issue 😉