-
-
Notifications
You must be signed in to change notification settings - Fork 10.8k
DOC: Fix for building with sphinx 3 #16370
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
|
|
@@ -370,7 +370,7 @@ Construction and Destruction | |||
arrays or structured arrays containing an object type) | |||
may be accepted and used in the iterator. If this flag | |||
is enabled, the caller must be sure to check whether | |||
:c:func:`NpyIter_IterationNeedsAPI(iter)` is true, in which case |
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.
Could maybe keep the argument by using :c:expr:
here
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.
Using :c:expr:
, this raises a warning: WARNING: c:identifier reference target not found: iter
. I think this reference should be removed (``NpyIter_IterationNeedsAPI(iter)``
).
Searching doesn't work. Maybe the cause is incompatibility of |
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.
There were a couple things that stood out to me from taking a quick look at this (I haven't made it all the way through yet):
- It seems that there are a lot of changes that fit the pattern of
:c:var:
->:c:macro:
. In these cases the macros don't have arguments, which according to the sphinx docs is supported in sphinx 2.X. If these work as expected, they could potentially be moved to DOC: Remove links for C codes #16896 so that they can be incorporated sooner. - Sphinx 3 adds the
:c:macro: name (arglist)
pattern. This means that many of the things that are currently defined using the.. :c:function:
directive can be switched over. Was there a reason you decided to stick with the.. :c:function:
approach instead?
There are now merge conflicts here |
Now there are warnings:
|
The cause of these warnings is that we use sphinx 2. I fixed
|
I still see the same warnings in the circleci : build job. Also you need to rebase or merge from master to fix the other bulid failures. |
This transition (from sphinx 2->3) seems like it will be tricky for this reason, since the doc builds are not going to be compatible between 2 & 3. I guess in this case we'd have to ignore the doc-build CI and run tests locally, or update the CI config as part of the PR (not sure if this change would get picked up though). |
@takanori-pskq will you be continuing with this? |
I think we should break this into two PRs: one to do the absolute minimum needed to get sphinx 3.2 to work, and another to update our C roles to a more modern sphinx. Note that sphinx 3.2 has a new |
And the warnings mentioned above ( |
a84bd43
to
4e8fba7
Compare
I think you need to add |
This page has become monstrous due to the |
Fixed the issue caused by
here |
Nice hack - although I don't like the use of
|
|
Theoretically, the "right" way to fix this is
However, having tried it in this circumstance, it doesn't appear to work (I could swear it worked in earlier versions but I haven't had time to dig into this). In any case, you've a neat hack for working around it. |
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 looking really good now, thanks! I think my only remaining nit is :c:type:
vs :c:expr
.
Edit: Agreed to deal with that in a later PR, #16370 (comment)
Note the
Which is almost two times worse than it was before. |
I found that there weren't many changes, so I fixed in this PR. |
The full set of warnings:
|
The The error is strange, because the Python intersphinx contains
|
Here, |
Is this ready now? |
Yes, it is. I don't know how to remove the "WIP" label. |
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.
FWIW I'm +1 on this. It seems like the sticking points (e.g. the increased warnings) shouldn't be blockers and could be dealt with in follow-up PRs, and getting the docs building on sphinx 3 is IMO worth the hiccups. It might be worth creating an issue for the :members:/:exclude-members:
hack so that we can clean that up when a working solution (hopefully :no-members:
) is available.
There's a lot of nice work in here, thanks @takanori-pskq!
Thanks @takanori-pskq. Hopefully there will not be much fallout from the transition. |
Related #16217
The warnings has decreased, but some warnings remain, and the generated document is still broken.