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

DOC: fix napoleon, render sphinx docs right #691

Merged

Conversation

mikofski
Copy link
Member

pvlib python pull request guidelines

Thank you for your contribution to pvlib python! You may delete all of these instructions except for the list below.

You may submit a pull request with your code at any stage of completion.

The following items must be addressed before the code can be merged. Please don't hesitate to ask for help if you're unsure of how to accomplish any of the items below:

  • Closes issue Read The Docs not rendering API arguments : types correctly #690
  • I am familiar with the contributing guidelines.
  • Fully tested. Added and/or modified tests to ensure correct behavior for all reasonable inputs. Tests (usually) must pass on the TravisCI and Appveyor testing services.
  • Updates entries to docs/sphinx/source/api.rst for API changes.
  • Adds description and name entries in the appropriate docs/sphinx/source/whatsnew file for all changes.
  • Code quality and style is sufficient. Passes LGTM and SticklerCI checks.
  • New code is fully documented. Includes sphinx/numpydoc compliant docstrings and comments in the code where necessary.
  • Pull request is nearly complete and ready for detailed review.

Brief description of the problem and proposed solution (if not already fully described in the issue linked to above):

  • add napoleon extension to sphinx source conf.py
  • fix see also in solar_zenith_analytical

@wholmgren
Copy link
Member

Thanks Mark. My understanding is that we should use either the napoleon extension or the numpydoc extension, but not both. I googled around a bit for pros/cons of napoleon with numpydoc formatting vs. just using numpydoc. I didn't find anything conclusive. It seems that switching from numpydoc to napoleon is trivial for some projects and difficult for others. We'd need to thoroughly check the build to make sure that all pages render correctly.

@mikofski
Copy link
Member Author

new docs as rendered are here

@wholmgren
Copy link
Member

Overall, I prefer the napoleon formatting. I also like that it links to the allowed types. I wonder if there's a way to link to custom definitions for "numeric" and "array-like" (not required here, just thinking).

I've listed a handful of issues below. Many of these are also issues in the numpydoc build, but the differences in formatting have made them breaking issues instead of nice to have fixes.

Things that are entirely broken in both and not required here:

@mikofski
Copy link
Member Author

mikofski commented Apr 23, 2019

  • Location.get_solarposition param kwargs
    just needed a newline and tab after kwargs
    image

  • new line needed after return type in solar_zenith_analytical
    image
    FYI: this is the expected rtype behavior for sphinx, setting napoleon_use_rtype = False changes this to:
    image

  • Location.get_clearsky kwargs spec is not sensible with napoleon.
    Just needs newline and tab after kwargs
    image

  • sapm returns section
    How do you want this to look? Separating rtype is the default Sphinx expected. I'll try napoleon_use_rtype = False maybe it looks better:
    image

  • SingleAxisTracker params not rendered right.
    It was missing the Parameters heading before the arguments list

  • ModelChain.prepare_inputs Assigns attributes list spilling to next line makes the section too confusing. Move it to notes?

  • bifacial parameters not rendered

  • SingleAxisTracker.singleaxis

@wholmgren
Copy link
Member

FYI: this is the expected rtype behavior for sphinx, setting napoleon_use_rtype = False changes this to:

ah, I prefer the behavior when set to False. Your preference?

@mikofski
Copy link
Member Author

It appears that the __init__ method has always been listed in the documentation, see v0.6.0, but I can't figure out how to get rid of it, things I've tried, but failed:

  • get rid of numpydoc extensions
  • set numpydoc directive numpydoc_show_class_members to false
  • set napoleon_include_init_with_doc to false
  • set napoleon_include_special_with_doc to false
  • set napoleon_include_private_with_doc to false
  • delete source/generated folder
  • hard cache reset the browser
  • add extra newline after Parameters section
    I give up :(

@mikofski
Copy link
Member Author

napolean directives I did not use:

# napoleon_google_docstring = False
# napoleon_use_param = False
# napoleon_use_ivar = True
# napoleon_include_init_with_doc = False
# napoleon_include_private_with_doc = False
# napoleon_include_special_with_doc = False

* set napoleon_use_rtype = False - for nicer style, combines return type
in parentheses after return name
* in bifacial use Parameters instead of Inputs
* in location use add newline and tab after kwargs parameter in two
places
* in model chain remove return and separate Notes
* in solarposition, add newline and tab after return type
* in tracking add Parameters to docstring for SingleAxisTracker
@mikofski
Copy link
Member Author

Make sure to reset browser cache before checking rendered docs

Copy link
Member

@wholmgren wholmgren left a comment

Choose a reason for hiding this comment

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

I like it! Minor comments and line too long errors...

docs/sphinx/source/conf.py Outdated Show resolved Hide resolved
pvlib/tracking.py Outdated Show resolved Hide resolved
* remove from
    - INSTALL REQUIRES in setup.py
    - extensions in conf.py, also remove numpydoc directive
    - tutorials environment yml
* fix typo for singleaxis in tracking.py
* stickler CI
    - too long lines in tracking.py and modelchain.py
* fix some formatting, confusing markdown and rst code backticks

Signed-off-by: Mark Mikofski <bwana.marko@yahoo.com>
@wholmgren wholmgren added this to the 0.6.2 milestone Apr 24, 2019
Copy link
Member

@wholmgren wholmgren left a comment

Choose a reason for hiding this comment

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

Thanks Mark!

@wholmgren wholmgren merged commit 2e844a5 into pvlib:master Apr 24, 2019
@mikofski mikofski deleted the fix_see_also_docs_solar_zenith_analytical branch April 24, 2019 22:46
@kandersolar kandersolar mentioned this pull request Jul 19, 2023
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants