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

Silence sphinx warnings #4286

Merged
merged 57 commits into from Aug 19, 2020
Merged

Conversation

keewis
Copy link
Collaborator

@keewis keewis commented Jul 29, 2020

This prepares our docstrings for the improvements to napoleon in the upcoming sphinx release (I'm not aware of a fixed date, though it's going to be released around Aug 08, so in about a week from now). napoleon now has a stricter syntax for parameter type specs with napoleon_use_param = True, which allows it to link to all referenced types.

We can't do much about the inherited docstrings, though, so we'd probably have to fix that upstream.

  • works towards closing Hundreds of Sphinx errors #3370
  • Passes isort . && black . && mypy . && flake8
  • User visible changes (including notable bug fixes) are documented in whats-new.rst
  • New functions/methods are listed in api.rst

@keewis
Copy link
Collaborator Author

keewis commented Jul 31, 2020

@spencerkclark, while working on this PR I got a warning about CFTimeOffset not being linkable in CFTimeIndex.floor / ceil / round. Looking through the code, I can only find BaseCFTimeOffset, there's no CFTimeOffset class (maybe we should rename that base class?).

Anyway, I don't think any of those is part of the public api? If the methods on CFTimeIndex are public, I think we either shouldn't reference these classes or make them public (or add a glossary term to define what CFTimeOffset means).

@spencerkclark
Copy link
Member

Anyway, I don't think any of those is part of the public api? If the methods on CFTimeIndex are public, I think we either shouldn't reference these classes or make them public (or add a glossary term to define what CFTimeOffset means).

Thanks for catching this; I agree they are not currently public API, so let's remove the references to them in docstrings of public functions. And it's true, I think CFTimeOffset would be a better name for the base class, but we can wait until later to act on that.

@max-sixty
Copy link
Collaborator

Thanks a lot @keewis !

@keewis keewis mentioned this pull request Aug 8, 2020
@dcherian
Copy link
Contributor

@keewis: this is great!

image

Comment on lines 100 to 108
name
The names of dimensions, coordinates, DataArray objects and data
variables can be anything as long as they are :term:`hashable`. However,
it is preferred to use :py:class:`str` typed names.

scalar
By definition, a scalar is not :term:`array_like`. That means that,
e.g., :py:class:`int`, :py:class:`float`, and :py:class:`str` values are
"scalar" while :py:class:`list` or :py:class:`tuple` are not.
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

these definitely need a review, I'm not quite sure these are easy to understand

@keewis
Copy link
Collaborator Author

keewis commented Aug 12, 2020

most of the remaining broken links are due to other sections (mostly the See Also section). To fix that, napoleon has to first add support for preprocessing those sections.

Copy link
Member

@shoyer shoyer 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 a great cleanup, thank you!

it is preferred to use :py:class:`str` typed names.

scalar
By definition, a scalar is not :term:`array_like`. That means that,
Copy link
Member

Choose a reason for hiding this comment

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

Should "array like" also be defined somewhere in order for a link to work? Or does this just add some sort of formatting?

My definition of a "scalar" might be anything that isn't an array and that when converted into a NumPy array would have 0 dimensions.

NumPy defines "array like" as anything that can be converted into an array. So that definition includes scalar values.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

right now we're linking to numpy's definition of the term (thanks to intersphinx), so defining it on our own might not be necessary.

I updated the definition, is it better now?

doc/terminology.rst Outdated Show resolved Hide resolved
doc/terminology.rst Outdated Show resolved Hide resolved
Co-authored-by: Maximilian Roos <5635139+max-sixty@users.noreply.github.com>
variables can be anything as long as they are :term:`hashable`. However,
it is preferred to use :py:class:`str` typed names.

scalar
Copy link
Contributor

Choose a reason for hiding this comment

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

LGTM.

doc/dask.rst Show resolved Hide resolved
@dcherian
Copy link
Contributor

Thanks @keewis. Great work!

@dcherian dcherian merged commit d9ebcaf into pydata:master Aug 19, 2020
@keewis keewis deleted the silence-sphinx-warnings branch August 19, 2020 15:01
@max-sixty
Copy link
Collaborator

A Napoleonic PR! (pun intended)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants