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

Fix type preprocessor #8021

Merged
merged 16 commits into from Aug 4, 2020
Merged

Fix type preprocessor #8021

merged 16 commits into from Aug 4, 2020

Conversation

keewis
Copy link
Contributor

@keewis keewis commented Jul 31, 2020

Follow-up to #7690. While trying to prepare a project I'm working on for the type preprocessor, I noticed a few issues (which was to be expected, I guess):

  • Ellipsis and ... are not treated like singletons
  • the Returns, Yields and Raises (and possibly more) fields ignore the napoleon_type_aliases setting (Edit: see Preprocess other sections #8050)
  • there are more delimiters in use, for example , or , , and , and and to (for describing the structure of a dictionary)

and possibly more. I'll find and fix those while working on that project.

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.

LGTM with nits.

BTW, could you merge the HEAD of 3.x into your branch please? It is needed to fix the CI error.

sphinx/ext/napoleon/docstring.py Outdated Show resolved Hide resolved
@@ -846,8 +848,13 @@ def combine_set(tokens):

def _tokenize_type_spec(spec: str) -> List[str]:
def postprocess(item):
if item.startswith("default"):
return [item[:7], item[7:]]
if item.startswith("default") and item != "default":
Copy link
Member

Choose a reason for hiding this comment

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

I noticed this would match "defaultdict" unexpectedly. How about using item.startswith("default ") (append a space after "default") instead?

Copy link
Contributor Author

@keewis keewis Aug 1, 2020

Choose a reason for hiding this comment

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

oh, yes, I didn't consider that. However, when attempting to add the default syntax to the numpydoc format guide someone reminded me that there are lots of projects with different formats, so right now we're trying to come up with a way to resolve this. I think for now it would be good not to restrict the syntax too much. How about something like

Suggested change
if item.startswith("default") and item != "default":
if re.match(r"^default[^_0-9a-zA-Z].*$", item):

Copy link
Member

Choose a reason for hiding this comment

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

As I commented on the last PR, we should not enhance numpydoc itself. We don't need to support dialects of numpydoc on napoleon. I don't object to choose an unrestricted way. But it does not mean supporting them all.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

if you look at numpy/numpydoc#289, it seems the format for default is not standardized at all, so we're considering mentioning three of them (default x, default: x and default=x) and state that there might be more. I was thinking of explicitly supporting those three.

Copy link
Member

Choose a reason for hiding this comment

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

No reason to support non-standardized notations. Let see what happens in numpy/numpydoc#289.
It would be better to split a PR to around "default" and others.

Copy link
Contributor Author

@keewis keewis Aug 4, 2020

Choose a reason for hiding this comment

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

fair enough. This might be something that should be discussed as part of that "default" PR, but what do you think about removing : from _token_regex (which I think we shouldn't support if not directly after default) and use something like:

pattern = re.compile(
    r"^"
    r"(default)"
    r"([^_0-9A-Za-z]+)"
    rf"({_xref_regex}|(?:[_A-Za-z][_A-Za-z0-9]*))?"
    r"$"
)
match = pattern.match(item)
if match is not None:
    return [_ for _ in match.groups() if _ is not None]
else:
    return [item]

in the postprocessing step? That way we "accidentally" support all the default<delimiter><obj> notations but can still decide to officially support / test only a limited set of notations.

Copy link
Member

Choose a reason for hiding this comment

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

In my experience, users who depend on such "accidental" features sometimes report a bug when it will be lost in the unexpected change.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

agreed. I guess we should continue this discussion once the numpydoc format guide PR was merged.

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 accept the change with pleasure after numpydoc guide updated :-)

sphinx/ext/napoleon/docstring.py Outdated Show resolved Hide resolved
@tk0miya tk0miya merged commit fcf63a2 into sphinx-doc:3.x Aug 4, 2020
@tk0miya
Copy link
Member

tk0miya commented Aug 4, 2020

Merged. Thank you!

@keewis keewis deleted the fix-type-preprocessor branch August 4, 2020 15:20
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jul 25, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants