Skip to content

Consolidate docs substitutions#11599

Merged
mhvk merged 5 commits into
astropy:mainfrom
nstarman:docs_unify_subs
Apr 20, 2021
Merged

Consolidate docs substitutions#11599
mhvk merged 5 commits into
astropy:mainfrom
nstarman:docs_unify_subs

Conversation

@nstarman

Copy link
Copy Markdown
Member

Description

The docs repeatedly define ReST substitutions for Astropy classes like |Quantity| or |Table|. This PR consolidates them into the docs/conf.py::rst_epilog so substitutions need only be defined once, and can also be used in code docstrings.

Fixes #11063

Signed-off-by: Nathaniel Starkman (@nstarman) <nstarkman@protonmail.com>
@nstarman

Copy link
Copy Markdown
Member Author

@pllim. Purely cosmetic, so push to next milestone?

@nstarman

nstarman commented Apr 19, 2021

Copy link
Copy Markdown
Member Author

Alternatively, I can definitely do this for Friday.

@pllim

pllim commented Apr 19, 2021

Copy link
Copy Markdown
Member

This looks uncontroversial enough that it might still make it into 4.3. Though, we might want to document that this is how we want the refs to be done in the dev doc for... doc.

@nstarman

nstarman commented Apr 19, 2021

Copy link
Copy Markdown
Member Author

I'll add a note to that effect.

looks like pytest sub is flaky

Signed-off-by: Nathaniel Starkman (@nstarman) <nstarkman@protonmail.com>
@nstarman

Copy link
Copy Markdown
Member Author

Darn. Pytest doc sub is flaky. Will try to fix.

@mhvk mhvk left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another really nice clean-up action! I have only one request, which is not to include OrderedDict in the list - maybe the best argument is just to stick to astropy classes only...

Comment thread docs/table/access_table.rst Outdated
Comment thread docs/table/construct_table.rst Outdated
@mhvk

mhvk commented Apr 19, 2021

Copy link
Copy Markdown
Contributor

@taldcroft, @adrn - as two "randomly" chosen others to have a look at this: do you agree with the concept of centralizing aliases used in the docs? I've reviewed this and the implementation is all OK with me (modulo a small change easily fixed), so if you agree that the idea is sound, I'll merge.

@adrn

adrn commented Apr 19, 2021

Copy link
Copy Markdown
Member

I agree with the concept! Unfortunately don't have time to review this in detail today, maybe tomorrow...

@taldcroft

Copy link
Copy Markdown
Member

I agree with the concept.

@mhvk

mhvk commented Apr 19, 2021

Copy link
Copy Markdown
Contributor

@adrn, @taldcroft - no need to review, as I think I was thorough enough; the agreement with the concept was what I was looking for!

@nstarman - ping me when you removed the OrderedDict short-cut and we'll get this in! But also do check the readthedocs failure - looks related!

@nstarman

nstarman commented Apr 20, 2021

Copy link
Copy Markdown
Member Author

@mhvk. Ok. I finally managed to resolve the RTD issues with pytest. Turns out it was very similar to https://thomas-cokelaer.info/blog/2020/04/sphinx-raise-a-warning-duplicate-explicit-target-name/. I'm going to be pushing my changes a few pieces at a time to make sure I resolved the behavior and didn't break any docs links. I'll ping you when it's all done!

Signed-off-by: Nathaniel Starkman (@nstarman) <nstarkman@protonmail.com>
Signed-off-by: Nathaniel Starkman (@nstarman) <nstarkman@protonmail.com>
Need to anonymize astropy#11118 link

Signed-off-by: Nathaniel Starkman (@nstarman) <nstarkman@protonmail.com>
@nstarman

Copy link
Copy Markdown
Member Author

Hunting down that problem was a fair bit of work because adding subs to rst_epilog appends them to every file and tox -e build_docs doesn't show you the specific file that raises the error when rst_epilog has the sub and the file has a non-anonymous link of the same name. I'm trying to find a place in the documentation to add a note about this. @mhvk, any suggestions?

@nstarman

Copy link
Copy Markdown
Member Author

Ok, @mhvk, except for a possible note in the dev docs, this is G2G!

@nstarman

nstarman commented Apr 20, 2021

Copy link
Copy Markdown
Member Author

I see a lot of potential for followup:

  • More link consolidation. I was pretty conservative: only collapsing ReST substitutions and transcluded files.
  • Replacing near-duplicated links with their rst_epilog counterparts.
  • using the links in docstrings. Here, I almost successfully avoided touching anything not in the docs.

@mhvk

mhvk commented Apr 20, 2021

Copy link
Copy Markdown
Contributor

@nstarman - OK, let's get this in! Probably good to have separate issue(s) for the follow-up. My overall sense is to keep it relatively limited, as you have it here, and otherwise err on the side of having references close to where they are needed.

@mhvk mhvk left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great, thanks!!!

@mhvk mhvk merged commit 199873d into astropy:main Apr 20, 2021
@nstarman nstarman deleted the docs_unify_subs branch April 20, 2021 22:22
@embray

embray commented Apr 22, 2021

Copy link
Copy Markdown
Member

Thank you for doing this. I wonder if we could move all of these into a separate file from conf.py though? I feel like it's a big chunk of the file now and makes it harder to read. It's also not super clear that it's a place now to put common links to use throughout the docs.

I propose moving it out to something like docs/common_links.txt that can be read in by conf.py.

@mhvk

mhvk commented Apr 22, 2021

Copy link
Copy Markdown
Contributor

Separating it out sounds good.

@nstarman

Copy link
Copy Markdown
Member Author

@embray . I like the idea. I had kept the substitutions grouped by subpackages, but still all squashed together. This will allow them to be a little more separate.

@nstarman

Copy link
Copy Markdown
Member Author

@embray @mhvk, I opened #11624

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

RST Substitutions in docstrings

6 participants