Skip to content
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

Set up ReST substitutions for docs #1147

Merged
merged 26 commits into from
May 29, 2021
Merged

Conversation

namurphy
Copy link
Member

This PR follows the example of Astropy in setting up substitutions in docs (see their docs/common_links.txt and docs/conf.py files). Instead of having ~plasmapy.particles.particle_class.Particle, we can use |Particle|.

This PR adapts code from Astropy but we can do that since we have a copy of Astropy's license.

@github-actions github-actions bot added the docs PlasmaPy Docs at http://docs.plasmapy.org label May 20, 2021
@codecov
Copy link

codecov bot commented May 20, 2021

Codecov Report

Merging #1147 (2bc1969) into master (a51e3a6) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1147   +/-   ##
=======================================
  Coverage   96.98%   96.98%           
=======================================
  Files          71       71           
  Lines        7022     7022           
=======================================
  Hits         6810     6810           
  Misses        212      212           
Impacted Files Coverage Δ
plasmapy/particles/particle_class.py 99.10% <ø> (ø)
plasmapy/particles/particle_collections.py 99.02% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a51e3a6...2bc1969. Read the comment docs.

@rocco8773 rocco8773 self-assigned this May 20, 2021
@github-actions github-actions bot added the plasmapy.particles Related to the plasmapy.particles subpackage label May 20, 2021
This commit only uses the ReST substitutions for a class within the
docstrings for that class and the methods/attributes of that class.
@namurphy namurphy marked this pull request as ready for review May 20, 2021 23:38
@namurphy
Copy link
Member Author

namurphy commented May 21, 2021

  • The substantive things to review are docs/conf.py and docs/common_links.py.
  • The other changes are mostly:
    • Doing things like `~plasmapy.particles.particle_class.Particle`|Particle|
    • Adjusting paragraphs so that the lines are ≲72 characters as per PEP8
  • @rocco8773, @Tiger-Du, and I had discussed about whether or not to include notation like |Particle| in docstrings and didn't reach consensus, except that we both agreed that it would be reasonable to include that notation in the docstrings within |Particle|. So, I limited changes within plasmapy.particles docstrings to cases like this.
  • I changed the flake8 settings so that it doesn't check for undefined substitutions in docstrings since it didn't seem to find the substitutions defined in the separate file.
  • I mostly did this for plasmapy.particles and decided to stop because I'm finding this to be kind of fun and was risking some scope creep.

@namurphy
Copy link
Member Author

Stuff I still need to do:

  • Add a discussion of this into the documentation guide in the dev guide.

@namurphy namurphy changed the title Set up how to use replacements in docs Set up ReST substitutions for docs May 21, 2021
@rocco8773 rocco8773 self-requested a review May 28, 2021 21:27
docs/common_links.txt Outdated Show resolved Hide resolved
@@ -0,0 +1,60 @@
.. These are ReST substitutions and links that can be used throughout the docs
Copy link
Member

Choose a reason for hiding this comment

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

I think this should be a .rst file and not a .txt file. It's already formatted as a RST file and my IDE is going mad with RST commands in a normal txt file.

Copy link
Member Author

Choose a reason for hiding this comment

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

I just tried changing that and got an error that the substitution for CustomParticle is being defined twice. I think what's happening is that this is being treated as a source file now when it shouldn't be. A little less likely is that rst_epilog...which contains the contents of this file...is being appended to this file. I'm attempting to add this file name to exclude_patterns in conf.py...

Comment on lines +3 to +4
|Particle| Class
****************
Copy link
Member

Choose a reason for hiding this comment

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

I'm surprised substitutions in headings don't produce build warnings due to the length of underline being shorter than the heading after the substitution. I'm really curious what's going on behind the scenes here, but I'm going to resist going down the rabbit hole.

Copy link
Member Author

Choose a reason for hiding this comment

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

Given my experience with rabbit holes, we should probably rename them into dragon holes.

@@ -71,7 +72,7 @@ accessed using `~plasmapy.particles.standard_atomic_weight`.
<Quantity 207.2 u>
Copy link
Member

Choose a reason for hiding this comment

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

I can't directly comment on the line, but go up 3 lines from here and up the xref needs .atomic. added to it...

~plasmapy.particles.atomic.standard_atomic_weight

Copy link
Member Author

@namurphy namurphy May 29, 2021

Choose a reason for hiding this comment

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

Looks like there are a few others in that file. I'll learn my lesson and start a separate PR with a well-defined scope. Edited to add: #1158.

.. _install-pip:

Installation with pip
---------------------

To install the most recent release of PlasmaPy on `PyPI`_
with `pip <https://pip.pypa.io/en/stable/>`_ into an existing Python environment
with `pip`_ into an existing Python environment
Copy link
Member

@rocco8773 rocco8773 May 29, 2021

Choose a reason for hiding this comment

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

If we add pip to the intersphinx_mappings dict, then `pip` will likely automatically cross-reference.

Just a thought, I'm fine with it either way.

Copy link
Member Author

Choose a reason for hiding this comment

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

I tried adding pip to intersphinx_mappings but the link didn't work, so I reverted the change rather than go down a dragon hole. I'd be okay with this change at some future time.

changelog/1147.doc.rst Outdated Show resolved Hide resolved
Copy link
Member

@rocco8773 rocco8773 left a comment

Choose a reason for hiding this comment

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

This is looking good. Definitely makes the narrative documentation more readable.

Just address the following comments and then this gets my approval.

Co-authored-by: Erik Everson <eteverson@gmail.com>
Copy link
Member

@rocco8773 rocco8773 left a comment

Choose a reason for hiding this comment

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

🚀🥳

@namurphy namurphy merged commit 092e32b into PlasmaPy:master May 29, 2021
@namurphy namurphy deleted the doc-replacements branch May 29, 2021 01:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs PlasmaPy Docs at http://docs.plasmapy.org plasmapy.particles Related to the plasmapy.particles subpackage
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants