-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
Unpin Sphinx #14093
Unpin Sphinx #14093
Conversation
👋 Thank you for your draft pull request! Do you know that you can use |
Example of the new warnings with this PR:
|
@adrn , can we simplify |
Hm, for the warnings in coordinates, is this because the |
@saimn or @larrybradley might remember... |
See #14115. |
@saimn - can you re-open this? I fixed |
I reopened it though this might need a rebase by now... |
I thought this PR was not needed/useful, do we still need |
I'm using |
I think I may have it down to one that I don't quite understand, but we'll see. Also, need to check whether |
OK, I was wrong -- still lots in |
Ok, so at least one of the issues with the coordinates stuff is the inheritance diagram, i.e. it generates a load of errors, I shall hunt down all the others and figure it out. |
omg it passed. |
@@ -566,3 +566,35 @@ Reference/API | |||
============= | |||
|
|||
.. automodapi:: astropy.coordinates | |||
:skip: ICRS |
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 obviously isn't great (it's the whole contents of the __all__
). I don't think it substantially detracts from the documentation of this page (although maybe it needs a note to say things aren't listed here?). Ideally of course we would have a way of excluding objects on namespace, i.e. do something like :exclude-namespace: astropy.coordinates.builtin_frames
but 🤷♂️
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.
As long as we add a line that says "go here for frame classes" or something, I think this is acceptable if it is the only way we can unpin Sphinx max version.
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 classes are listed in the block above, so it's not that hard to spot but I am happy to add a line.
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.
Maybe good to see if we can change the transformgraph view to not link to classes - that seems to be the problem (and even now we cannot click on it anyway)
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.
Is there actually a problem, other than the links inside the graph not working? (I am not sure they did to begin with)
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 thought so for a bit, but now am really unsure what is actually causing what problems. Really, these many skip
shouldn't be necessary...
@@ -509,11 +510,12 @@ def __init__(self, *args, **kwargs): | |||
|
|||
@classmethod | |||
def get_roll0(cls): |
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.
Unrelated... but we might need a get_roll20
🤓
@@ -335,7 +335,9 @@ | |||
"reference_url": { | |||
"astropy": None, | |||
"matplotlib": "https://matplotlib.org/stable/", | |||
"numpy": "https://numpy.org/doc/stable/", | |||
# The numpy search js isn't loadable at the moment (2022-12-07) |
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.
Wait, what?
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 was causing an error, and it's because the JS is missing some quotes apparently 🤷♂️
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.
But I don't see any such failure in other PRs. Is the JS only used for this PR?
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.
Oooo is the JS only used if you unpin Sphinx? Is this something we have to report to numpy?
@@ -97,8 +97,9 @@ all = | |||
fsspec[http]>=2022.8.2 | |||
s3fs>=2022.8.2 | |||
docs = | |||
sphinx<4 | |||
sphinx |
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.
Also change the Jinja2 pin back to this:
Jinja2>=3.0
Nice!!! I think now you narrowed it down we should be able to get rid of the problems with the graph. It seems like we do not really need |
Given all the work Simon has poured into making noindex work, I feel like we are obligated to use 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.
Note that Sphinx 5.2 introduced a new feature (that is on by default) with a table of contents for functions and classes. That text overflows the left side bar window in the astropy sphinx theme. See, e.g., from this PR: https://astropy--14093.org.readthedocs.build/en/14093/api/astropy.table.Table.html#astropy.table.Table
Fortunately, Sphinx 5.2.3 added a configuration option to turn it off: set toc_object_entries = False
in docs.conf
.
xref: astropy/photutils#1424
xref: https://github.com/astropy/photutils/pull/1465/files
I think I've got an alternative without |
as suggested by larrybradley
I pushed a commit to address #14093 (review) . Thanks, @larrybradley ! |
See #14140 for my alternative -- I think it is cleaner. |
Closed in favour of #14140 |
Ref #11725 and astropy/sphinx-automodapi#150
So basically using
:noindex:
with astropy/sphinx-automodapi#150 works in the sense that it removes the duplication warnings from Sphinx. But then for Astropy we get other warnings/errors about missing references ...