Skip to content

Conversation

@steff456
Copy link
Member

@steff456 steff456 commented Jan 26, 2023

This PR,

  • Adds admonitions for changed APIs in 2022 version in the draft folder
  • Adds admonitions for new APIs in 2022 version in the draft folder

I guided myself using the changelog for both the changed and new APIs.

Some questions,

  1. Should I only add the admonitions for the draft version? Or do we also want them in the 2022 spec?
  2. When I compile locally, I get some sphinx errors with the new 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 😉

@steff456 steff456 added the Maintenance Bug fix, typo fix, or general maintenance. label Jan 26, 2023
@steff456 steff456 added this to the v2023 milestone Jan 26, 2023
@steff456 steff456 requested review from kgryte and rgommers January 26, 2023 00:00
@steff456 steff456 self-assigned this Jan 26, 2023
@rgommers
Copy link
Member

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 __dlpack__):

image

Should I only add the admonitions for the draft version? Or do we also want them in the 2022 spec?

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.

When I compile locally, I get some sphinx errors with the new 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 😉

I don't see an issue. Maybe also ensure you delete the build dir and the auto-generated files, that can cause problems sometimes.

Copy link
Member

@rgommers rgommers left a 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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Maintenance Bug fix, typo fix, or general maintenance.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants