Opened 4 years ago

Closed 4 years ago

#23017 closed enhancement (fixed)

Specifics of docstrings style

Reported by: klee Owned by:
Priority: minor Milestone: sage-8.0
Component: documentation Keywords:
Cc: Merged in:
Authors: Kwankyu Lee Reviewers: John Palmieri, Daniel Krenn
Report Upstream: N/A Work issues:
Branch: f752318 (Commits) Commit: f752318c90be9132d603449504cfedd20a74f940
Dependencies: Stopgaps:

Description (last modified by jdemeyer)

Some details of the Sage docstring styles are not clearly stated in the developer manual, and left to each writer's discretion. This introduced inconsistencies and sometimes allowed bad styles. The purpose of this ticket is to spot those details and provide specific guidelines for them, based on the consensus of the developer community.

The guidelines collected here will be used to revise

http://doc.sagemath.org/html/en/developer/coding_basics.html#documentation-strings


H1. Write

``True``, ``False``, ``None``

but do not write

True, False, None

This was discussed at https://groups.google.com/forum/#!topic/sage-devel/EVG7wQExYOQ


H2. It is preferable to write

if the lattice is reflexive

than

if ``self`` is reflexive

This was discussed at https://groups.google.com/forum/#!topic/sage-devel/dm8PQbni0Y8


H3. Write

Return whether this lattice is reflexive.

but do not write

Return ``True`` if the lattice is reflexive and ``False`` otherwise.

This was discussed at https://groups.google.com/forum/#!topic/sage-devel/OFuYydUMMzI


H4. OUTPUT block is optional but recommended unless it is clear from the one-line explanation.

This was discussed at https://groups.google.com/forum/#!topic/sage-devel/i9pemQvEE5E


H5. Write

OUTPUT: a tuple of lattices

This was discussed at https://groups.google.com/forum/#!topic/sage-devel/bdLBvSlJLPo


H6. Write

INPUT: 

- ``n`` -- integer (default: 1); the number of repetition

This was discussed at https://groups.google.com/forum/#!topic/sage-devel/eMkeLj_w9ZY

Change History (40)

comment:1 Changed 4 years ago by klee

  • Description modified (diff)

comment:2 Changed 4 years ago by klee

  • Description modified (diff)

comment:3 Changed 4 years ago by klee

  • Description modified (diff)

comment:4 Changed 4 years ago by jdemeyer

I would like to add that INPUT and OUTPUT blocks should be sufficiently verbose.

Some places in Sage have something like

INPUT:

- ``x`` -- integer

Well, that's not very useful. If should explain the purpose of x, not just the fact that it's an integer.

comment:5 Changed 4 years ago by jdemeyer

It might also be good to formally define the style of types and defaults in INPUT blocks. I usually use

INPUT:

- ``p`` -- (integer, default: `11`) the modulus for the modular algorithm

comment:6 Changed 4 years ago by jdemeyer

  • Description modified (diff)

comment:7 Changed 4 years ago by tscrim

Strong -1 to G1. True and similar should be code because they are Python objects.

Strong -1 to G2. self is a well-defined concept in Python, more concise, and more clear when subclasses are involved.

Subsequently, I see no good options in G3. However, option 3 is closet, and I prefer:

Return whether ``self`` is reflexive.

G4: recommended but optional. This is what we primarily do in practice anyways.

+1 on comment:5, although I do a slightly different style

INPUT:

- ``p`` -- integer (default: `11`); the modulus for the modular algorithm

comment:8 Changed 4 years ago by klee

  • Description modified (diff)

comment:9 follow-up: Changed 4 years ago by mforets

have you considered here the guidelines for writing an exception string?

no beginning upper case, no final full stop is what i've been told, but i just quickly checked documentation strings chapter and there are seemingly no guidelines.

only: “It is an error to ..

thanks

comment:10 in reply to: ↑ 9 ; follow-up: Changed 4 years ago by klee

Replying to mforets:

have you considered here the guidelines for writing an exception string?

No. This ticket is about docstrings.

If you have specific ideas, you may start your own ticket and polls about exception strings. But do you really have specific ideas to guide developers on exception strings other than "no beginning upper case, no final full stop"? Hmm, I don't have...

comment:11 in reply to: ↑ 10 Changed 4 years ago by mforets

Replying to klee:

Replying to mforets:

have you considered here the guidelines for writing an exception string?

No. This ticket is about docstrings.

If you have specific ideas, you may start your own ticket and polls about exception strings. But do you really have specific ideas to guide developers on exception strings other than "no beginning upper case, no final full stop"? Hmm, I don't have...

sure. the point was that "no beginning upper case, no final full stop" was sth that i learnt here in trac, which is ok, but better if it is documented somewhere close to the Guide (if i didn't miss it).

comment:12 Changed 4 years ago by klee

  • Authors set to Kwankyu Lee
  • Branch set to u/klee/23017

comment:13 Changed 4 years ago by git

  • Commit set to 18ebc5e9373a83e47828dbe1daf1cbd7dac623e7

Branch pushed to git repo; I updated commit sha1. New commits:

18ebc5eAdd some specific dosctring guides

comment:14 Changed 4 years ago by klee

  • Description modified (diff)

comment:15 Changed 4 years ago by git

  • Commit changed from 18ebc5e9373a83e47828dbe1daf1cbd7dac623e7 to 92812ec387990d3cc14d666632bd260589bfc6e5

Branch pushed to git repo; I updated commit sha1. New commits:

92812ecThis is the final one

comment:16 Changed 4 years ago by git

  • Commit changed from 92812ec387990d3cc14d666632bd260589bfc6e5 to 21fcb16df242406de3558de43ee4fb023f33c19d

Branch pushed to git repo; I updated commit sha1. New commits:

21fcb16Modify the template

comment:17 follow-ups: Changed 4 years ago by dkrenn

  • Cc dkrenn added
  • Reviewers set to Daniel Krenn

1)

+       The lexicographic product of G and H is the graph with vertex set ...

use ... of `G` and `H` (or ... of ``G`` and ``H``) for correct typesetting here. (I prefer `G`.)

2)

+       - ``p`` -- prime integer (default: 2); coprime with ``n``

use (default: `2`) (or (default: ``2``)) for correct typesetting. (I prefer `2`.)

Change this in the template (lines +540,28) as well.

3)

+       OUTPUT: a tuple of

shouldn't this be a tuple ``(H, U)`` of ?

4) More a question, as I am still compiling my current SageMath: Is the link in

+processed by Sphinx. See http://www.sphinx-doc.org/rest.html for an

typeset correctly?

I did not review the bibliography-part carefully, but were there any changes or was it simply changeing indent?

comment:18 Changed 4 years ago by dkrenn

5)

+- Lines should be shorter than 80 characters.

Here one could link to the PEP8 https://www.python.org/dev/peps/pep-0008/#maximum-line-length

comment:19 in reply to: ↑ 17 Changed 4 years ago by klee

Replying to dkrenn:

I did not review the bibliography-part carefully, but were there any changes or was it simply changeing indent?

Just changing the indent.

comment:20 in reply to: ↑ 17 Changed 4 years ago by klee

Replying to dkrenn:

4) More a question, as I am still compiling my current SageMath: Is the link in

+processed by Sphinx. See http://www.sphinx-doc.org/rest.html for an

typeset correctly?

Yes.

comment:21 follow-up: Changed 4 years ago by jmantysalo

  • Cc dkrenn removed
  • Reviewers Daniel Krenn deleted

Template has a .. NOTE:: block of what should be an ALGORITHM:. Not sure if that should be changed in this ticket or not.

comment:22 Changed 4 years ago by git

  • Commit changed from 21fcb16df242406de3558de43ee4fb023f33c19d to d2c86769df83fd3e3c157d35725d51b9e1e20f4c

Branch pushed to git repo; I updated commit sha1. New commits:

d2c8676Minor fixes

comment:23 in reply to: ↑ 17 Changed 4 years ago by klee

Replying to dkrenn:

1)

+       The lexicographic product of G and H is the graph with vertex set ...

use ... of `G` and `H` (or ... of ``G`` and ``H``) for correct typesetting here. (I prefer `G`.)

Done.

2)

+       - ``p`` -- prime integer (default: 2); coprime with ``n``

use (default: `2`) (or (default: ``2``)) for correct typesetting. (I prefer `2`.)

Change this in the template (lines +540,28) as well.

Done.

3)

+       OUTPUT: a tuple of

shouldn't this be a tuple ``(H, U)`` of ?

I thinks "a tuple of" suffices here.

comment:24 in reply to: ↑ 21 Changed 4 years ago by klee

Replying to jmantysalo:

Template has a .. NOTE:: block of what should be an ALGORITHM:. Not sure if that should be changed in this ticket or not.

This ticket will not touch that. I think that block is close to a note as it does not explain the algorithm itself.

comment:25 Changed 4 years ago by jdemeyer

  • Description modified (diff)

comment:26 Changed 4 years ago by klee

  • Status changed from new to needs_review

comment:27 Changed 4 years ago by git

  • Commit changed from d2c86769df83fd3e3c157d35725d51b9e1e20f4c to 2e0d0f8ad8a7571ae2adb1662ef19305dddb0aa1

Branch pushed to git repo; I updated commit sha1. New commits:

2e0d0f8Add PEP8

comment:28 follow-up: Changed 4 years ago by jdemeyer

If the OUTPUT block is "optional but recommended", shouldn't the same be true for the INPUT block? I am thinking of this docstring for example:

cpdef edge_connectivity(g):
    r"""
    Computes the edge connectivity of the input graph, using Boost.

    OUTPUT: a pair ``(ec, edges)``, where ``ec`` is the edge
    connectivity, ``edges`` is the list of edges in a minimum cut.

Here, there is no explicit INPUT but I don't think it's needed anyway.

comment:29 Changed 4 years ago by git

  • Commit changed from 2e0d0f8ad8a7571ae2adb1662ef19305dddb0aa1 to 3325c92383282b9d4457e0b3a0f4ad7371534de8

Branch pushed to git repo; I updated commit sha1. New commits:

3325c92INPUT block is not "not optional"

comment:30 in reply to: ↑ 28 Changed 4 years ago by klee

Replying to jdemeyer:

If the OUTPUT block is "optional but recommended", shouldn't the same be true for the INPUT block?

Certainly there are many cases that INPUT block is not needed, and in practice we do not add one in such cases. So not optional is too much prohibitive. I just removed the sentence.

comment:31 follow-up: Changed 4 years ago by jhpalmieri

If you want to follow the guidelines here (all Sphinx directives are written in uppercase), then you should change ".. code-block::" to ".. CODE-BLOCK::". You should also change it from ".. code-block:: none" to ".. CODE-BLOCK:: rest".

The bulleted point starting with "Lines should be shorter than 80 characters." has a line which is exactly 80 characters, so perhaps that should be broken up so all lines are shorter than 80 characters. Some of the other lines (for example in the paragraph starting "The decomposition of the free module ..." are now longer than 80 characters.

Why did you delete "If it contains multiple lines, all the lines after the first need to begin at the same indentation as the backtick"? I think that is helpful information, and people often make mistakes with this indentation in docstrings.

comment:32 Changed 4 years ago by git

  • Commit changed from 3325c92383282b9d4457e0b3a0f4ad7371534de8 to 00253ce209f02469ac881776ac1a6c9a17fbd623

Branch pushed to git repo; I updated commit sha1. New commits:

c3aaf3bMerge branch 'develop' into docstring_trac23017
00253ceFixes to follow the guidelines

comment:33 in reply to: ↑ 31 Changed 4 years ago by klee

Replying to jhpalmieri:

If you want to follow the guidelines here (all Sphinx directives are written in uppercase), then you should change ".. code-block::" to ".. CODE-BLOCK::". You should also change it from ".. code-block:: none" to ".. CODE-BLOCK:: rest".

Done.

The bulleted point starting with "Lines should be shorter than 80 characters." has a line which is exactly 80 characters, so perhaps that should be broken up so all lines are shorter than 80 characters. Some of the other lines (for example in the paragraph starting "The decomposition of the free module ..." are now longer than 80 characters.

I broke up many lines to make it shorten than 80 characters. But I kept some of the lines as they look better as they are than broken up. This includes the second line of the bulleted point, which has exactly 80 characters and the 80th character is a period. I think this is a good example showing that a guideline is just a guideline and not to be followed blindly when there are good reasons not to follow :-)

Why did you delete "If it contains multiple lines, all the lines after the first need to begin at the same indentation as the backtick"? I think that is helpful information, and people often make mistakes with this indentation in docstrings.

Recovered.

comment:34 Changed 4 years ago by jhpalmieri

I would suggest one more change:

  • src/doc/en/developer/coding_basics.rst

    diff --git a/src/doc/en/developer/coding_basics.rst b/src/doc/en/developer/coding_basics.rst
    index 51adfc1b3e..1f7f4a0f3a 100644
    a b introduction. Sage imposes these styles: 
    507507- All reST and Sphinx directives (like ``.. WARNING::``, ``.. NOTE::``,
    508508  ``.. MATH::``, etc.) are written in uppercase.
    509509
    510 - Code fragments are quoted with ``````. This includes function arguments
    511   and the Python literals like ````True````, ````False```` and ````None````.
     510- Code fragments are quoted with double backticks. This includes
     511  function arguments and the Python literals like ````True````,
     512  ````False```` and ````None````. For example:
     513
     514  .. CODE-BLOCK:: rest
     515
     516      If ``check`` is ``True``, then ...
    512517
    513518Sage's master **BIBLIOGRAPHY** file
    514519^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

Otherwise, it is okay with me.

comment:35 Changed 4 years ago by git

  • Commit changed from 00253ce209f02469ac881776ac1a6c9a17fbd623 to f752318c90be9132d603449504cfedd20a74f940

Branch pushed to git repo; I updated commit sha1. New commits:

f752318Add example for code fragments

comment:36 Changed 4 years ago by jhpalmieri

  • Reviewers set to John Palmieri
  • Status changed from needs_review to positive_review

Okay, thank you.

comment:37 Changed 4 years ago by dkrenn

  • Reviewers changed from John Palmieri to John Palmieri, Daniel Krenn

comment:38 Changed 4 years ago by dkrenn

Fine for me as well (it seems that I was removed from the reviewers field; I've added my name again; all my suggestions above were incoperated.)

comment:39 Changed 4 years ago by klee

Thank you!

comment:40 Changed 4 years ago by vbraun

  • Branch changed from u/klee/23017 to f752318c90be9132d603449504cfedd20a74f940
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.