-
Notifications
You must be signed in to change notification settings - Fork 128
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
docs: add CI, fix existing warnings, add other improvements #1095
Conversation
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.
Nice to get these checked and fixed!
docs/conf.py
Outdated
# Don't check for type references in docstrings. | ||
nitpick_ignore_regex = [ | ||
('py:.*', '.*'), | ||
] | ||
|
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 smells stinky to me. It's very broad, and the error messages we're getting (as pasted in the commit message) don't seem to make sense. For example:
docstring of augur.align.prepare:1: WARNING: py:class reference target not found: The existing alignment filename
Why is the string The existing alignment filename
being used as a reference target (e.g. a link destination)?
Something's fishy here. Is the sphinx.ext.napoleon
conversion (63297d4) doing something bogus? Are a bunch of our docstrings using the wrong syntax for describing parameters?
I think it's worth tracking down what's going on here, as I suspect papering over it now will only cause us more headache in the future.
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've replaced 8005577 with several fixes: 0c3cb34...4375322
This ended up being a doozy, but I learned lots about how these docs are generated!
1b4de33
to
67cc7c8
Compare
Codecov ReportPatch and project coverage have no change.
Additional details and impacted files@@ Coverage Diff @@
## master #1095 +/- ##
=======================================
Coverage 68.25% 68.25%
=======================================
Files 63 63
Lines 6772 6772
Branches 1659 1659
=======================================
Hits 4622 4622
Misses 1841 1841
Partials 309 309
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report in Codecov by Sentry. |
d383e35
to
ae81fdd
Compare
31b0f41
to
9211721
Compare
This is required for docs-ci to work properly. It is already done in docs.nextstrain.org and ncov: https://github.com/nextstrain/docs.nextstrain.org/blob/5b82daa7d6f9f784e93bdef967af101f91fd2c75/Makefile#L4-L7 https://github.com/nextstrain/ncov/blob/822a77ba070d1d37483d45c22a00b5f83d16e974/docs/Makefile#L4-L7
This uses a reusable workflow in the nextstrain/.github repo.
rST is our standard doc format; Markdown is the legacy format. Initial conversion performed with: pandoc -f markdown-smart -t rst-smart docs/faq/metadata.md > docs/faq/metadata.rst and then I hand reviewed and made additional edits. This fixes a broken link to https://docs.nextstrain.org/en/latest/guides/bioinformatics/lat_longs.html.
The page was renamed in nextstrain/docs.nextstrain.org@a3b3856.
This was causing a warning when building the docs: Tests ----- None:8: CRITICAL: Unexpected section title. Removing since docstrings in other files do not have this heading.
In CLI pages with multiple headings.
Replace the literal blocks (introduced by the :: markers) with code-block directives for nice syntax highlighting.
Fixes this warning: augur/io/__init__.py:docstring of augur.io:1: WARNING: duplicate object description of augur.io, other instance in api/developer/augur.io, use :noindex: for one of them While :noindex: for Sphinx in general disables cross-referencing¹ and it is briefly mentioned in the "Options and advanced usage" section of automodule², there aren't any references anyways (as demonstrated by a lack of related warnings/errors). However, it's still unclear to me exactly why this shows on augur.io only. I assume it has something to do with the re-exporting of public API functions, which is currently specifi to augur.io. ¹ https://www.sphinx-doc.org/en/master/usage/restructuredtext/domains.html#basic-markup ² https://www.sphinx-doc.org/en/master/usage/extensions/autodoc.html#directives
The section headings resulted in warnings: None:8: CRITICAL: Unexpected section title. Comparison methods ================== None:36: CRITICAL: Unexpected section title. Distance maps ============= They rendered fine in the Development API page¹ but did not render on the CLI page². This seems like a bug with sphinx-argparse³. For now, use bold text to avoid the warnings and allow the text to render on the CLI page. ¹ https://docs.nextstrain.org/projects/augur/en/stable/api/developer/augur.distance.html ² https://docs.nextstrain.org/projects/augur/en/stable/usage/cli/distance.html ³ sphinx-doc/sphinx-argparse#31
This makes local builds show the same warnings in CI without erroring.
Without this prefix, a warning appears: WARNING: py:attr reference target not found: ValidationMode.SKIP
The majority of the codebase uses numpydoc for type annotations. However, there are some usages of the newer PEP 484 type hints. For example, augur.dates.get_numerical_dates has `metadata:pd.DataFrame` in the function signature. Without sphinx-autodoc-typehints, this caused a warning: augur/augur/dates.py:docstring of augur.dates.get_numerical_dates:1: WARNING: py:class reference target not found: pandas.core.frame.DataFrame This commit fixes that warning.
This enables proper reference resolution during the docs build, and provides useful links on the generated doc pages.
Without these pages, there would be warnings such as: WARNING: py:class reference target not found: AugurError WARNING: py:class reference target not found: DataErrorMethod It's a good idea to have these doc pages regardless.
np.isscalar will return true for a float, but that is not valid input for np.linspace(num). Instead, allow native or numpy integers.
Otherwise the doctest gets parsed as numpydoc.
Better than the previous commit since this section is made for doctest examples¹. ¹ https://numpydoc.readthedocs.io/en/v1.5.0/format.html#examples
The bracket notation is suggested by PyCharm¹ but is not standard numpydoc. ¹ https://www.jetbrains.com/help/pycharm/type-syntax-for-docstrings.html
"label" is not a valid type; it should be "str".
These resulted in nitpick "reference target not found" warnings.
Other register_parser functions don't have a docstring, and this one is out of date.
These were causing nitpick warnings: WARNING: py:class reference target not found: TYPE
Bio.Phylo is the module, not the tree class. This could instead be something more specific like Bio.Phylo.Newick.Tree, but I opted for the more generic type.
`Phylo.Node` is not a valid class. I believe the intention is `Bio.Phylo.BaseTree.Clade`, since that is the type of `Bio.Phylo.BaseTree.Tree.root`
This class lives at pandas.io.parsers.TextFileReader, however, there are no public docs for it and it isn't linked on pandas doc pages either¹. Use single backticks to disable linking while denoting that it is a class². ¹ https://pandas.pydata.org/pandas-docs/version/1.4/reference/api/pandas.read_csv.html ² https://numpydoc.readthedocs.io/en/latest/format.html#common-rest-concepts
9211721
to
8e3705d
Compare
I spot-checked the preview build and it looks good. Merging this to start checking docs in other CI runs. Post-merge feedback is still welcome. |
Description of proposed changes
I surfaced warnings in the docs build via CI and fixed them. Also added some additional touch-ups from going through these docs files.
Related issue(s)
Testing
Checklist