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

DM-13755: ReST-ify config doc strings #21

Merged
merged 2 commits into from
Mar 15, 2018

Conversation

SimonKrughoff
Copy link
Contributor

This changes how the default Field constructor handles the doc argument. It also changes how ChoiceField and RangeField handle self.__doc__.

Example base Field, RangeField and ChoiceField:

 |  doWriteExposure
 |      Write icExp and icExpBackground in addition to icSrc? Ignored if doWrite False. (`bool`)
 |  
 |  psfIterations
 |      Number of iterations of detect sources, measure sources, estimate PSF. If useSimplePsf is True then 2 should be plenty; otherwise more may be wanted. (`int`)
 |      
 |      Valid Range = [1,inf)
 |
 |  flatScalingType
 |      The method for scaling the flat on the fly. (`str`)
 |      
 |      Allowed values:
 |      
 |      USER
 |        Scale by flatUserScale
 |      MEAN
 |        Scale by the inverse of the mean
 |      MEDIAN
 |        Scale by the inverse of the median
 |      None
 |        Field is optional

@parejkoj
Copy link
Contributor

This looks great, thanks for the quick fix. Two comments from me (not official reviewer):

  1. How hard would it be to also add the default value to the resulting docstring?
  2. I discovered this while converting jointcal. You can test to see if you get errors about the astrometryModel/photometryModel Config parametesr in jointcal. Run sphinx-build . _build/html from within doc/ on branch /u/parejkoj/sphinx-docs (make sure you've setup and sconsed it).

@jonathansick
Copy link
Member

I managed to test this with the jointcal sphinx-docs branch. astrometryModel and photometryModel work, as far as I can tell.

screenshot-2018-3-13 jointcalconfig jointcal

@parejkoj
Copy link
Contributor

Great, thanks! Looking at that output, my only suggestion would be to wrap the allowed values in monospace or italics or something, to more clearly distinguish them from the description text. But otherwise this looks pretty good.

@jonathansick
Copy link
Member

jonathansick commented Mar 13, 2018

Exactly, for those choice fields, it might be good if the value was marked up as a code literal:

``'simplePoly'``
    One Polynomial per ccd.

I think you could do something like

'``{0!r}``'.format(value)

To let Python add the quotes around string values.

@jonathansick
Copy link
Member

I really like the idea of including defaults. Do you think this is do-able in this ticket @SimonKrughoff?

The interesting this is that with regular class attributes, numpydoc includes the default value from the initial assignment. But these config field docstrings behave more like properties.

I just noticed that our numpydoc spec doesn't spell out where to but default values. https://developer.lsst.io/docs/py_docs.html#documenting-class-properties

One option is to put it next to the type info on the summary line, like this:

"""Summary sentence (<type>, default <default).
"""

Do we have really complicated defaults? In that case we'd want to put it in a new paragraph of hte extended summary:

"""Summary sentence ().

Default::

<default>

"""

@SimonKrughoff
Copy link
Contributor Author

Let me spend a couple of minutes adding the ticks and defaults.

Copy link
Member

@jonathansick jonathansick left a comment

Choose a reason for hiding this comment

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

This is terrific, thanks @SimonKrughoff

@SimonKrughoff SimonKrughoff changed the title ReST-ify config doc strings DM-13755: ReST-ify config doc strings Mar 15, 2018
@SimonKrughoff SimonKrughoff merged commit ae1598d into master Mar 15, 2018
@ktlim ktlim deleted the u/krughoff/fix_docstrings branch August 25, 2018 05:50
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.

3 participants