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

Preprocess other sections #8050

Merged
merged 11 commits into from Oct 29, 2020
Merged

Conversation

keewis
Copy link
Contributor

@keewis keewis commented Aug 4, 2020

Follow-up to #7690, depends on #8049. This enables the type processor for other sections (Returns, Yields and Raises, possibly more) and also adds tests.

Turns out that the reason why I thought the previous PR didn't work was that I added the call to the preprocessor in the wrong place.

Related to #8004, but for numpydoc instead of google.

@keewis
Copy link
Contributor Author

keewis commented Aug 4, 2020

I just noticed this doesn't work for Raises because you can't have a role in :raises:. Do you have any suggestions on how to deal with that, @tk0miya? To be clear, the exceptions don't have to be preprocessed, only translated to a different namespace. E.g. if we have

Raises
------
MergeError
    failed to merge
OtherError
    description

and

napoleon_type_aliases = {
    ...
    "MergeError": "~pandas.errors.MergeError",
    "OtherError": ":py:exc:`~mypackage.OtherError",
    ...
}

it should become

:raises ~pandas.errors.MergeError: failed to merge
:raises ~mypackage.OtherError: description

@keewis
Copy link
Contributor Author

keewis commented Aug 6, 2020

not sure if this is worth it, but this change makes sure roles are always stripped, even when they're around invalid names. That means we don't have to make the type preprocessor more complicated than it already is, with the disadvantage that it adds roles that will be stripped directly afterwards.

Edit: tests will work after merging #8049

@tk0miya tk0miya added this to the 3.3.0 milestone Aug 7, 2020
@keewis
Copy link
Contributor Author

keewis commented Aug 7, 2020

so now that #8049 is merged, the tests pass, too. Is there a reason why you scheduled this for 3.3.0?

@tk0miya
Copy link
Member

tk0miya commented Aug 7, 2020

I don't have time for reviewing until release now. I have other topics for 3.2 release. Is this more important than others?

@keewis
Copy link
Contributor Author

keewis commented Aug 7, 2020

not really, but it extends the type preprocessor to other fields (Returns, Yields, Raises), so it would fit pretty well with the other PRs related to that. Also, the diff is pretty small because the reason why it didn't work before was that I added the call to the preprocessor in the wrong place.

If you're too busy I guess it's fine to push this to 3.3.0

@tk0miya
Copy link
Member

tk0miya commented Aug 7, 2020

Sorry for waiting. But I need to read the napoleon extension before reviewing because I'm a newbie of the module. So I need time for reviewing even for a minor fix.

@keewis
Copy link
Contributor Author

keewis commented Aug 7, 2020

no worries. Also, thanks for the quick reviews on the other PRs.

@keewis
Copy link
Contributor Author

keewis commented Aug 18, 2020

gentle ping, @tk0miya. Are there any updates on this?

@keewis
Copy link
Contributor Author

keewis commented Sep 3, 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.

LGTM!

@tk0miya tk0miya merged commit 68c89f4 into sphinx-doc:3.x Oct 29, 2020
tk0miya added a commit that referenced this pull request Oct 29, 2020
@keewis keewis deleted the preprocess-other-sections branch October 29, 2020 17:32
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jul 22, 2021
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

2 participants