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 )
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
- Description modified (diff)
comment:2 Changed 4 years ago by
- Description modified (diff)
comment:3 Changed 4 years ago by
- Description modified (diff)
comment:4 Changed 4 years ago by
comment:5 Changed 4 years ago by
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
- Description modified (diff)
comment:7 Changed 4 years ago by
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
- Description modified (diff)
comment:9 follow-up: ↓ 10 Changed 4 years ago by
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: ↓ 11 Changed 4 years ago by
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
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
- Branch set to u/klee/23017
comment:13 Changed 4 years ago by
- Commit set to 18ebc5e9373a83e47828dbe1daf1cbd7dac623e7
Branch pushed to git repo; I updated commit sha1. New commits:
18ebc5e | Add some specific dosctring guides
|
comment:14 Changed 4 years ago by
- Description modified (diff)
comment:15 Changed 4 years ago by
- Commit changed from 18ebc5e9373a83e47828dbe1daf1cbd7dac623e7 to 92812ec387990d3cc14d666632bd260589bfc6e5
Branch pushed to git repo; I updated commit sha1. New commits:
92812ec | This is the final one
|
comment:16 Changed 4 years ago by
- Commit changed from 92812ec387990d3cc14d666632bd260589bfc6e5 to 21fcb16df242406de3558de43ee4fb023f33c19d
Branch pushed to git repo; I updated commit sha1. New commits:
21fcb16 | Modify the template
|
comment:17 follow-ups: ↓ 19 ↓ 20 ↓ 23 Changed 4 years ago by
- 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
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
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
comment:21 follow-up: ↓ 24 Changed 4 years ago by
- 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
- Commit changed from 21fcb16df242406de3558de43ee4fb023f33c19d to d2c86769df83fd3e3c157d35725d51b9e1e20f4c
Branch pushed to git repo; I updated commit sha1. New commits:
d2c8676 | Minor fixes
|
comment:23 in reply to: ↑ 17 Changed 4 years ago by
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 ofshouldn'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
Replying to jmantysalo:
Template has a
.. NOTE::
block of what should be anALGORITHM:
. 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
- Description modified (diff)
comment:26 Changed 4 years ago by
- Status changed from new to needs_review
comment:27 Changed 4 years ago by
- Commit changed from d2c86769df83fd3e3c157d35725d51b9e1e20f4c to 2e0d0f8ad8a7571ae2adb1662ef19305dddb0aa1
Branch pushed to git repo; I updated commit sha1. New commits:
2e0d0f8 | Add PEP8
|
comment:28 follow-up: ↓ 30 Changed 4 years ago by
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
- Commit changed from 2e0d0f8ad8a7571ae2adb1662ef19305dddb0aa1 to 3325c92383282b9d4457e0b3a0f4ad7371534de8
Branch pushed to git repo; I updated commit sha1. New commits:
3325c92 | INPUT block is not "not optional"
|
comment:30 in reply to: ↑ 28 Changed 4 years ago by
Replying to jdemeyer:
If the
OUTPUT
block is "optional but recommended", shouldn't the same be true for theINPUT
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: ↓ 33 Changed 4 years ago by
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
- Commit changed from 3325c92383282b9d4457e0b3a0f4ad7371534de8 to 00253ce209f02469ac881776ac1a6c9a17fbd623
comment:33 in reply to: ↑ 31 Changed 4 years ago by
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
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: 507 507 - All reST and Sphinx directives (like ``.. WARNING::``, ``.. NOTE::``, 508 508 ``.. MATH::``, etc.) are written in uppercase. 509 509 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 ... 512 517 513 518 Sage's master **BIBLIOGRAPHY** file 514 519 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
Otherwise, it is okay with me.
comment:35 Changed 4 years ago by
- Commit changed from 00253ce209f02469ac881776ac1a6c9a17fbd623 to f752318c90be9132d603449504cfedd20a74f940
Branch pushed to git repo; I updated commit sha1. New commits:
f752318 | Add example for code fragments
|
comment:36 Changed 4 years ago by
- Reviewers set to John Palmieri
- Status changed from needs_review to positive_review
Okay, thank you.
comment:37 Changed 4 years ago by
- Reviewers changed from John Palmieri to John Palmieri, Daniel Krenn
comment:38 Changed 4 years ago by
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
Thank you!
comment:40 Changed 4 years ago by
- Branch changed from u/klee/23017 to f752318c90be9132d603449504cfedd20a74f940
- Resolution set to fixed
- Status changed from positive_review to closed
I would like to add that
INPUT
andOUTPUT
blocks should be sufficiently verbose.Some places in Sage have something like
Well, that's not very useful. If should explain the purpose of
x
, not just the fact that it's an integer.