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

preprocessing numpy types #7690

Merged
merged 51 commits into from Jul 25, 2020

Conversation

keewis
Copy link
Contributor

@keewis keewis commented May 17, 2020

Subject: napoleon should translate the type fields

Feature or Bugfix

  • Feature
  • Bugfix

could be both?

Purpose

As reported in #6861, napoleon simply puts everything after the colon of a numpy-style parameter description (e.g. the int in val : int) into a :type: field. This works most of the time, but numpydoc also uses certain keywords (optional, default), value sets (e.g. {"val1", "val2"}) and literals (as in default: "val2"), among others. Right now, using those will raise nit-picky warnings and possibly also result in incorrect parameter documentation.

This PR adds a new role (named noref, but could also be literal or text; I'm not confident in my ability to come up with good names) that is then used to make the type field ignore the keywords / literals above.

The type preprocessing also allows adding a mapping of terms to other links (see the translations dict), so terms like dict-like or array-like / array_like can be mapped to mapping using a configuration option. It isn't restricted to terms, though, any kind of role should be possible.

I don't really know if this is the best approach to the problem described in #6861 (in that issue, the suggestion to create a new section for optional parameters came up) but I'm submitting this to start making progress.

Relates

Copy link
Member

@tk0miya tk0miya left a comment

Choose a reason for hiding this comment

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

Your idea is great! LGTM!

sphinx/ext/napoleon/docstring.py Outdated Show resolved Hide resolved
sphinx/ext/napoleon/docstring.py Outdated Show resolved Hide resolved
sphinx/ext/napoleon/docstring.py Outdated Show resolved Hide resolved
sphinx/ext/napoleon/docstring.py Outdated Show resolved Hide resolved
sphinx/ext/napoleon/docstring.py Outdated Show resolved Hide resolved
sphinx/ext/napoleon/docstring.py Outdated Show resolved Hide resolved
@tk0miya tk0miya added this to the 3.1.0 milestone May 29, 2020
tests/test_ext_napoleon_docstring.py Outdated Show resolved Hide resolved
sphinx/ext/napoleon/docstring.py Outdated Show resolved Hide resolved
@keewis
Copy link
Contributor Author

keewis commented Jun 4, 2020

gentle ping, @tk0miya.

As a summary, this is what I think still needs to be answered / done (see also above):

  • what is the default role used by the :type: field?
  • how do I add location information to the warnings?
  • how do I modify the test so that I can verify that a malformed value set emits the warning (something like self.assertWarnsRegex or pytest.warns that works with the logger)?
  • the warning tends to be emitted multiple times when using autosummary / autodoc. Is that something I can fix?
  • regarding continuation lines: I adopted the recommendation in numpydoc (it seems that we'd need to modify the rst standard on definition blocks if we wanted to do something else). Any objections?
  • when extending a library using inheritance, the docstrings of inherited methods often contain references to other objects within the same namespace (without specifying the qualname). Is there a existing way to specify packages that should be checked if it can't be found in the current namespace?
  • I just noticed default doesn't have a standardized format. pandas seems to mostly use default <default> but with the current implementation that is really difficult to support (but not impossible). Should I try to get this to work?

@keewis keewis marked this pull request as ready for review June 6, 2020 00:11
@tk0miya tk0miya modified the milestones: 3.1.0, 3.2.0 Jun 6, 2020
@keewis
Copy link
Contributor Author

keewis commented Jul 2, 2020

gentle ping, @tk0miya

Copy link
Member

@tk0miya tk0miya left a comment

Choose a reason for hiding this comment

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

Sorry for very very late response. I'm back from the maintenance of 3.1.x bug fixes now.

what is the default role used by the :type: field?

By default, :class: role is used. As an exception, :obj: is used for None value.

how do I add location information to the warnings?

Unfortunately there no good way. The autodoc-process-docstring that napoleon uses does not send a location of the docstring. It might be able to use self._obj to obtain the name of the docstring owner.

how do I modify the test so that I can verify that a malformed value set emits the warning (something like self.assertWarnsRegex or pytest.warns that works with the logger)?

Please see my comment below.

the warning tends to be emitted multiple times when using autosummary / autodoc. Is that something I can fix?

I don't have an answer. I need to investigate what happened in this case. Can I reproduce that in this test case? If true, I'll take a look.

regarding continuation lines: I adopted the recommendation in numpydoc (it seems that we'd need to modify the rst standard on definition blocks if we wanted to do something else). Any objections?

Of course, yes. no problem. The NumpyDocstring class has followed to numpydoc format.

when extending a library using inheritance, the docstrings of inherited methods often contain references to other objects within the same namespace (without specifying the qualname). Is there a existing way to specify packages that should be checked if it can't be found in the current namespace?

This is another problem. In my memory is correct, a similar issue was already filed (I can't find it now...).

I just noticed default doesn't have a standardized format. pandas seems to mostly use default but with the current implementation that is really difficult to support (but not impossible). Should I try to get this to work?

I don't know about pandas at all. So I can't determine that.

In addition, the goal of napoleon (I supposed) is not supporting all of the formats for docstrings. They are only for google's and numpy's docstring. So no reason to support it.

sphinx/ext/napoleon/docstring.py Outdated Show resolved Hide resolved
sphinx/ext/napoleon/docstring.py Outdated Show resolved Hide resolved
sphinx/ext/napoleon/docstring.py Show resolved Hide resolved
sphinx/ext/napoleon/docstring.py Show resolved Hide resolved
sphinx/ext/napoleon/docstring.py Outdated Show resolved Hide resolved
sphinx/ext/napoleon/docstring.py Outdated Show resolved Hide resolved
tests/test_ext_napoleon_docstring.py Outdated Show resolved Hide resolved
tests/test_ext_napoleon_docstring.py Outdated Show resolved Hide resolved
Copy link
Contributor Author

@keewis keewis left a comment

Choose a reason for hiding this comment

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

By default, :class: role is used. As an exception, :obj: is used for None value.

That still doesn't get a consistent link style: the docstring of the test project's test.multiply results in

:param a: first factor
:type a: :term:`array-like <array_like>`
:param b: second factor
:param \*variables: parameters without use
:type \*variables: str
:param mod: optionally compute the modulo of the product
:type mod: :term:`array-like <array_like>`, *optional*
:param files: save each value of the results into a new file
:type files: :class:`list` of :class:`str` or :class:`list` of :class:`os.PathLike` or :term:`dict-like <mapping>`
:param save_format: file format
:type save_format: ``{"ma{icious", "options", "that need a line break"}``, *default*: ``"options"``
:param test: optional parameter that does absolutely nothing
:type test: *optional*
:param \*\*variable_kwargs: kwargs form of variables

but the rendered version (logs) displays certain objects as preformatted link. Is there any way to make that a plain link, just like the single str for *variables? Or should I just leave this as-is?

I don't have an answer. I need to investigate what happened in this case. Can I reproduce that in this test case? If true, I'll take a look.

Unfortunately not by the test case, but with the project linked above. I guess that's because I'm using both autosummary and autodoc?

Edit: I can't reproduce it anymore. Not sure what happened, but I guess it's fixed?

Unfortunately there no good way. The autodoc-process-docstring that napoleon uses does not send a location of the docstring. It might be able to use self._obj to obtain the name of the docstring owner.

Thanks, that works; now only the line number is missing. Since all other warnings about docstrings also seem to be missing the line number I guess that's not so bad.

This is another problem. In my memory is correct, a similar issue was already filed (I can't find it now...).

I'll try to find that issue.

Edit: I can't find it either. However, I can work around that issue using the new napoleon_type_aliases setting, so this shouldn't be a blocker.

I don't know about pandas at all. So I can't determine that.

In addition, the goal of napoleon (I supposed) is not supporting all of the formats for docstrings. They are only for google's and numpy's docstring. So no reason to support it.

pandas uses numpydoc, but extends it slightly to allow documenting default values as part of the type. I'm also proposing to extend numpydoc, with a slightly different syntax (default: None instead of pandas' default None). Is it okay to include this for now (and ask on the numpydoc issue tracker) or should I wait for a response before including that?

@keewis
Copy link
Contributor Author

keewis commented Jul 14, 2020

with this I think there's only a few issues left:

  • formatting the created links consistently (but do tell me if you think that's not an issue)
  • the namespacing for referenced objects (there might be another issue about that)
  • the extension of numpydoc (default: <value> is not part of numpydoc, but it also won't reject it)
  • am I merging into the correct branch? If napoleon_use_param is true there's a lot more warnings, not sure if that makes it a breaking change?

@tk0miya
Copy link
Member

tk0miya commented Jul 15, 2020

formatting the created links consistently (but do tell me if you think that's not an issue)

Sorry, my explanation above was not correct. It uses :class: and :obj: like role, but it uses emphasis node instead of literal. To be precise, they are not a role. The docfield mechanism creates the structure of nodes directly without roles.

And I can't find the way to build type descriptions having the same style.

the namespacing for referenced objects (there might be another issue about that)

I found issues related to this: #5977, #4961 and #272.

the extension of numpydoc (default: is not part of numpydoc, but it also won't reject it)
am I merging into the correct branch? If napoleon_use_param is true there's a lot more warnings, not sure if that makes it a breaking change?

At present, the napoleon extension officially supports only google style and numpy style docstrings. So it should not add a support non-official notation. I'll never accept to support non-numpydoc syntax as numpydoc. The only I can agree is making napoleon extendable from 3rd party's extension.

am I merging into the correct branch? If napoleon_use_param is true there's a lot more warnings, not sure if that makes it a breaking change?

Oh, sorry. 3.x branch is better to merge.

About napoleon_use_param, we have to support both enabled and disabled. It's not good to bring new warnings to existing users after adding a new feature. I don't think to drop the flag for now. So I think it is a bug, not a breaking change.

@keewis
Copy link
Contributor Author

keewis commented Jul 15, 2020

And I can't find the way to build type descriptions having the same style.

Hmm... well, the links work so while the current state is not ideal, this can be fixed in a new PR.

At present, the napoleon extension officially supports only google style and numpy style docstrings. So it should not add a support non-official notation. I'll never accept to support non-numpydoc syntax as numpydoc. The only I can agree is making napoleon extendable from 3rd party's extension.

So that means if I can get this to become official numpydoc we're fine?

Edit: see numpy/numpydoc#284

3.x branch is better to merge.

I'll change that

Edit: done

About napoleon_use_param, we have to support both enabled and disabled. It's not good to bring new warnings to existing users after adding a new feature. I don't think to drop the flag for now. So I think it is a bug, not a breaking change.

I changed the code so the type spec only gets parsed with napoleon_use_param = True, which I think is consistent with the current behavior?

About the warnings: those are the ones that are generated for wrong type specs (invalid numpydoc), e.g. if value sets are not closed, end quotes are missing or there's a missing space after a comma. Those warnings have been nitpicky before (because linking to a value set failed) so if you tell me how I can change these to nitpicky, I will do that.

@keewis
Copy link
Contributor Author

keewis commented Jul 21, 2020

I added support for the other style, too. With that, I guess the only thing left is to decide what to do with the warnings?

@keewis keewis requested a review from tk0miya July 24, 2020 18:54
sphinx/ext/napoleon/__init__.py Outdated Show resolved Hide resolved
sphinx/ext/napoleon/docstring.py Outdated Show resolved Hide resolved
sphinx/ext/napoleon/docstring.py Show resolved Hide resolved
@tk0miya
Copy link
Member

tk0miya commented Jul 25, 2020

I changed the code so the type spec only gets parsed with napoleon_use_param = True, which I think is consistent with the current behavior?

Thank you for your update. I can accept your patch now (with nits). On the other hand, it would be wonderful if we'll support numpy types even for napoleon_use_param = False. Either way, it would be better to separate it into another PR to merge this early.

About the warnings: those are the ones that are generated for wrong type specs (invalid numpydoc), e.g. if value sets are not closed, end quotes are missing or there's a missing space after a comma. Those warnings have been nitpicky before (because linking to a value set failed) so if you tell me how I can change these to nitpicky, I will do that.

I think it's acceptable warnings if the mark-ups are really broken. So it is not needed to change them nitpicky.

It seems numpydoc (the implementation, not the guide) supports both formats (default None and default: None) so I guess we probably should support both, too?

I'm not familiar with numpydoc. So I'll merged this as is. From the point of view of a maintainer, it would be better to describe (by comment, docstring, or something) why it is implemented if it is not a part of "official" spec.

Copy link
Contributor Author

@keewis keewis left a comment

Choose a reason for hiding this comment

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

why it is implemented if it is not a part of "official" spec

I think that's because they didn't update the docstring guide in quite a while. I'll try submitting a PR changing the docstring guide (the official spec).

it would be wonderful if we'll support numpy types even for napoleon_use_param = False.

Sure, but I guess that would change the current behavior of not linking at all.

sphinx/ext/napoleon/docstring.py Outdated Show resolved Hide resolved
sphinx/ext/napoleon/docstring.py Show resolved Hide resolved
@keewis
Copy link
Contributor Author

keewis commented Jul 25, 2020

done. I also changed the link to the official spec to its new location (the old one is just a in-text redirect).

@tk0miya
Copy link
Member

tk0miya commented Jul 25, 2020

I think that's because they didn't update the docstring guide in quite a while. I'll try submitting a PR changing the docstring guide (the official spec).

I think it should be documented to upstream's doc if they have special meaning for the colon after "default" keyword. So it sounds nice :-)

@keewis
Copy link
Contributor Author

keewis commented Jul 25, 2020

if they have special meaning for the colon after "default" keyword

the colon doesn't have a special meaning, it's completely optional. numpydoc accepts both formats (without mentioning any of them in the docstring guide), so I adapted this PR (see 9b42560) to do the same.

:param arg1: Description of `arg1`
:type arg1: mypackage.CustomType
:param arg2: Description of `arg2`
:type arg2: :term:`dict-like <mapping>`
Copy link
Member

Choose a reason for hiding this comment

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

.. versionadded:: 3.2 tag is needed at the end of description.

Copy link
Member

Choose a reason for hiding this comment

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

I'll update this after merging.

@tk0miya tk0miya merged commit ac0a800 into sphinx-doc:3.x Jul 25, 2020
@keewis keewis deleted the transform_numpy_parameter_types branch July 25, 2020 12:42
@tk0miya
Copy link
Member

tk0miya commented Jul 25, 2020

Just Merged. Thank you for your great work!

@embray
Copy link
Contributor

embray commented Jul 28, 2020

Thanks for this--much more sophisticated and complete than my hackish workaround.

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

Successfully merging this pull request may close these issues.

None yet

3 participants