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

feat: Add trim_doctest_flag to google and numpy parsers #134

Merged
merged 2 commits into from Feb 19, 2022

Conversation

thatlittleboy
Copy link
Contributor

@thatlittleboy thatlittleboy commented Feb 19, 2022

Related to: mkdocstrings/mkdocstrings#386

I've decided to start the work on the above issue starting from the legacy parser. Please take a look and see if I'm going in the right direction or if I've missed anything crucial.

Key PR pointers

  • Feat: Implemented the functionality of removal of # doctest: and <BLANKLINE> via regex; in both cases where 1. line starts with >>> and 2. line is "in a code example block". This is activated via the new trim_doctest_flags parameter / option.
  • As mentioned in the parent issue, I have set the default behaviour of trim_doctest_flags to be True. To keep consistent with Sphinx.
  • Only implemented for Google and Numpy for now. I'm not sure how to do it for Rst, it doesn't seem like Rst is parsing examples explicitly (or at all?). If there is a way to implement for Rst, I would be grateful if you could nudge me in the right direction. I'll try to incorporate it into this PR as well. (since our pyjanitor project is using the rst / legacy-python combination 😅 )
  • Tests: Added one test function specifically for testing the removal of doctest flags. I've set trim_doctest_flags=False as default argument in the test suite.
  • Docs: I've updated README (and took the liberty to reorganize the prose a little) to point out this new trim doctest option is available under docstring_options.

Addendum

Copy link
Member

@pawamoy pawamoy left a comment

Choose a reason for hiding this comment

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

Very, very nice! Thanks a lot!

@pawamoy
Copy link
Member

pawamoy commented Feb 19, 2022

You can rebase on main branch to fix Windows builds.

Only implemented for Google and Numpy for now. I'm not sure how to do it for Rst, it doesn't seem like Rst is parsing examples explicitly (or at all?). If there is a way to implement for Rst, I would be grateful if you could nudge me in the right direction. I'll try to incorporate it into this PR as well. (since our pyjanitor project is using the rst / legacy-python combination sweat_smile

Indeed, the RST parser does not even parse code examples. I will not ask you to add it, but if you want to do so, be my guest. Also, are you really using reStructuredText in your pyjanitor docstrings? I have to say that I strongly recommend switching to a style more akin to Markdown (google or even numpydoc): after all, you are generating docs with MkDocs, which uses Markdown exclusively 😕 Maybe you have this planned though, in that case, just ignore my comment 🙂

As to pointing you in the right direction if you want to try and implement code example parsing in the RST parser: I won't be of any help unfortunately. The RST parser was implemented by @plannigan.

@thatlittleboy
Copy link
Contributor Author

thatlittleboy commented Feb 19, 2022

@pawamoy

  1. Rebase on main? I don't see a main branch on the pytkdocs repo 😅
  2. Got it, thanks! Yeah the story with pyjanitor is that they switched over from Sphinx to mkdocs about last year (before I was around), I just supposed they stuck with rst for ease of transition. I think long-term the direction should be to switch over to the new parser, or at the very least switching over to google, as you mentioned. No worries then, this PR will just be targeted towards Google and Numpy. I think there'll be more value in porting this change over to griffe.

@pawamoy
Copy link
Member

pawamoy commented Feb 19, 2022

Arf, I thought I was on another repo, my bad! The main branch is master here indeed 🙂

I checked your docstrings: you're not really using RST I think, I just mislead myself by naming the parser "RST", when the :param name: syntax is just Sphinx-style I think. This is why I renamed this parser to sphinx in Griffe, less confusing.

@pawamoy
Copy link
Member

pawamoy commented Feb 19, 2022

The type check failed but I will fix it just after merging your PR if you don't mind.

@thatlittleboy
Copy link
Contributor Author

Sure, thanks!

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

2 participants