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

Docs: Enable autosummary for instrument module #4273

Merged
merged 3 commits into from
Jul 26, 2022

Conversation

jenshnielsen
Copy link
Collaborator

@jenshnielsen jenshnielsen commented Jun 16, 2022

This currently requires a branch of sphinx to fix documenting inherited attributes. That branch is open as sphinx-doc/sphinx#10691

To install sphinx 5.x we need the master branch of autodocsumm. That does not otherwise carry any fixes

This is done as a branch that carries my changes directly on top of 5.0.x since 5.1.0 broke the build. See sphinx-doc/sphinx#10701

@astafan8 What do you think is it acceptable to build with a patched version of sphinx until that is merged.

This will also enable us to activate autodocsumm for #4371 which will make that way more readable

@jenshnielsen jenshnielsen added the docs Related to docs improvements label Jun 16, 2022
@codecov
Copy link

codecov bot commented Jun 16, 2022

Codecov Report

Merging #4273 (b1b1ee7) into master (9d27254) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #4273   +/-   ##
=======================================
  Coverage   68.36%   68.36%           
=======================================
  Files         276      276           
  Lines       31024    31024           
=======================================
  Hits        21210    21210           
  Misses       9814     9814           

@jenshnielsen jenshnielsen changed the title Docs: Enable autosummary for insturment module Docs: Enable autosummary for instrument module Jun 16, 2022
@jenshnielsen jenshnielsen force-pushed the docs/instrument_summary branch 7 times, most recently from 0b2f7ae to 9d02e6d Compare July 15, 2022 17:58
@jenshnielsen jenshnielsen force-pushed the docs/instrument_summary branch 2 times, most recently from 47776d9 to 751ec08 Compare July 25, 2022 15:54
Copy link
Contributor

@astafan8 astafan8 left a comment

Choose a reason for hiding this comment

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

What do you think is it acceptable to build with a patched version of sphinx until that is merged

to keep things safe, i'd wait. i guess it all can easily take a month, right? the fix in sphinx is one thing, but also installing autodocsumm from master branch does not look super good.

what is the backup plan? we can just revert to a released version of sphinx and autodocsumm and remove the :autosummary: directive, right? if it's indeed that easy, then we can try to this risk, but perhaps pin autodocsumm to a particular commit?

@jenshnielsen
Copy link
Collaborator Author

to keep things safe, i'd wait. i guess it all can easily take a month, right? the fix in sphinx is one thing, but also installing autodocsumm from master branch does not look super good.

I would be happy to install it from a specific commit. The only reason that I did not bother is that its a project that moves fairly slow.

what is the backup plan? we can just revert to a released version of sphinx and autodocsumm and remove the :autosummary: directive, right? if it's indeed that easy, then we can try to this risk, but perhaps pin autodocsumm to a particular commit?

Yes, this can be undone by either removing the autosummary directive from modules that makes use of inherited attributes
or by allowing building the docs with warnings as errors in which case the inherited attributes will simply be missing from the tabels in the docs.

Copy link
Contributor

@astafan8 astafan8 left a comment

Choose a reason for hiding this comment

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

ok, with the backup plan laid out, let's go :)

@jenshnielsen jenshnielsen merged commit ba08fa6 into microsoft:master Jul 26, 2022
@jenshnielsen jenshnielsen deleted the docs/instrument_summary branch July 26, 2022 13:49
bors bot added a commit that referenced this pull request Aug 1, 2022
4371: Update agilent drivers to conform to docs standard r=jenshnielsen a=jenshnielsen

Updates to implement #4368  for Agilent. Closes #4365 

`@mgunyho` Could you test the new driver. I have updated the name of the variable and gotten rid of the not needed deg2rad converters and replaced them with numpy

I also suggest that we get rid of the autogenerated docs for the instruments and replace them with something like this.
I would like to add autodocsumm for these modules but am facing the same issue as in #4273

Co-authored-by: Jens H. Nielsen <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs Related to docs improvements
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants