Opened 6 years ago
Closed 6 years ago
#21646 closed enhancement (fixed)
Devel manual about docstrings
Reported by: | jmantysalo | Owned by: | |
---|---|---|---|
Priority: | major | Milestone: | sage-7.5 |
Component: | documentation | Keywords: | |
Cc: | tscrim, chapoton | Merged in: | |
Authors: | Jori Mäntysalo | Reviewers: | Jeroen Demeyer, Martin Rubey |
Report Upstream: | N/A | Work issues: | |
Branch: | f4818f0 (Commits, GitHub, GitLab) | Commit: | f4818f0987b3984f77e7d7e396a8a983de19dc13 |
Dependencies: | Stopgaps: |
Description
This patch will slightly modify instructions for docstrings.
I would like to have even more pre-specified formats for some classes of functions. For example should Boolean-valued is_something()
function be "Return True
if...", "Test whether" or something else? Also we have an arbitrary decision to use algorithm=
instead of implementation=
, method=
etc.
But as a first part, what you think about these changes?
Change History (41)
comment:1 Changed 6 years ago by
Branch: | → u/jmantysalo/devel-manual |
---|
comment:2 Changed 6 years ago by
Commit: | → ff653973cf1ec43e238a612ca81e97f3e2995f10 |
---|
comment:3 Changed 6 years ago by
Commit: | ff653973cf1ec43e238a612ca81e97f3e2995f10 → cb7c34c805dd3c20c25b549d95d23b71ae0140b3 |
---|
Branch pushed to git repo; I updated commit sha1. New commits:
cb7c34c | Minor addition.
|
comment:4 Changed 6 years ago by
Commit: | cb7c34c805dd3c20c25b549d95d23b71ae0140b3 → 49d57c85304078ae0a7fe49c3a2fd7d6e966f8f8 |
---|
Branch pushed to git repo; I updated commit sha1. New commits:
49d57c8 | Less duplication.
|
comment:5 Changed 6 years ago by
Milestone: | sage-7.4 → sage-7.5 |
---|---|
Status: | new → needs_review |
No comments got.
Anyways, this now encourages slightly more to use TESTS block. Also I documented some arbitrary decisions made.
comment:6 follow-up: 8 Changed 6 years ago by
Status: | needs_review → needs_work |
---|
- The example of a
TESTS
block is badly indented (it was already badly indented by 3 spaces only).
- I don't like advice of the form
Do not use ...
Be positive, say only what should be used.
comment:7 Changed 6 years ago by
Commit: | 49d57c85304078ae0a7fe49c3a2fd7d6e966f8f8 → f61ed30c2e39c257d96d20696ce5fb00788d0f6c |
---|
Branch pushed to git repo; I updated commit sha1. New commits:
f61ed30 | Minor changes.
|
comment:8 follow-up: 11 Changed 6 years ago by
Replying to jdemeyer:
- The example of a
TESTS
block is badly indented (it was already badly indented by 3 spaces only).
There were tabs. Corrected.
- I don't like advice of the form
Do not use ...
Be positive, say only what should be used.
How about this "instead of" -formulation?
comment:9 Changed 6 years ago by
Status: | needs_work → needs_review |
---|
comment:10 Changed 6 years ago by
There is still one more tab. Could you also make this grammatical fix:
-
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 60e49ec..d65e6d7 100644
a b information. You can use the existing functions of Sage as templates. 283 283 They should have good coverage of the functionality in question. 284 284 285 285 - A **SEEALSO** block (highly recommended) with links to related parts of 286 Sage. This helps users find the features that interest sthem and discover286 Sage. This helps users find the features that interest them and discover 287 287 the new ones. :: 288 288 289 289 .. SEEALSO::
comment:11 follow-up: 13 Changed 6 years ago by
comment:12 Changed 6 years ago by
Commit: | f61ed30c2e39c257d96d20696ce5fb00788d0f6c → 1b06af9127725ffc862acb9dc134be0e6768eaea |
---|
Branch pushed to git repo; I updated commit sha1. New commits:
1b06af9 | Reviewers comments.
|
comment:13 Changed 6 years ago by
Replying to jhpalmieri:
There is still one more tab. Could you also make this grammatical fix:
Arghs, Emacs. Tab removed and fix applied.
Replying to jdemeyer:
How about this "instead of" -formulation?
Why not simply say "Use X"?
Sohehow "Use algorithm
if there are several algorithms or backend programs to select from." sounds odd to me. It left me wondering what else I could use or what is the meaning of this sentence. Compare to "- - describes the function or method's effect as a command - - not as a description - -"
But as you wish, this is now changed too.
comment:14 follow-up: 15 Changed 6 years ago by
I was thinking that you could document a few standard keywords in the same way:
algorithm
: choose between various implementation (withNone
meaning a sensible default, which could depend on installed optional packages).
proof
: is this isTrue
, require a mathematically proven computation. WithFalse
, a probabilistic algorithm may be used.
check
: do some additional checks to verify the input parameters. This should not otherwise influence the functioning of the code (if code works withcheck=True
, it should also work withcheck=False
).
coerce
: convert the input parameters to a suitable parent. This is typically used in constructors. You can call a method withcoerce=False
to skip some checks if the parent is known to be correct.
- ...
comment:15 follow-up: 17 Changed 6 years ago by
Replying to jdemeyer:
I was thinking that you could document a few standard keywords in the same way:
This is exactly what I hope to have in this list; and for example inplace
should be added. But I still think that "Do not use"-list could be added; it will make this more explicit. An alternative for inplace
with default False
could be for example return_copy
with default True
, and that could be explained.
Has there been or suggested some other parameter name for coerce
?
comment:16 Changed 6 years ago by
Commit: | 1b06af9127725ffc862acb9dc134be0e6768eaea → edb1e67b15e78ae792701a190a0b33756fdbfa1f |
---|
Branch pushed to git repo; I updated commit sha1. New commits:
edb1e67 | Additions to keywords.
|
comment:17 follow-up: 18 Changed 6 years ago by
Replying to jmantysalo:
But I still think that "Do not use"-list could be added; it will make this more explicit.
As a separate list, it would already make more sense to me. That wouldn't clutter the original "keywords to use" list.
Has there been or suggested some other parameter name for
coerce
?
Not that I know, at least coerce
is used consistently for this. But I guess that convert
would be a better name.
comment:18 Changed 6 years ago by
Replying to jdemeyer:
Replying to jmantysalo:
But I still think that "Do not use"-list could be added; it will make this more explicit.
As a separate list, it would already make more sense to me. That wouldn't clutter the original "keywords to use" list.
Tried, but found no good way to express this as separate list.
Should we go with this patch?
comment:19 follow-up: 21 Changed 6 years ago by
Here is a proposal: replace
* Function parameter names:
by
* Common function keyword arguments: This is a list of some keyword arguments that many functions and methods take. For consistency, you should use the keywords from the list below with the meaning as explained here. Do not use a different keyword with the same meaning (for example, do not use ``method``; use ``algorithm`` instead).
comment:20 Changed 6 years ago by
Commit: | edb1e67b15e78ae792701a190a0b33756fdbfa1f → 8e027296608cc8998fd513ca33b35f22a5466a4f |
---|
Branch pushed to git repo; I updated commit sha1. New commits:
8e02729 | Explanation of common keywords.
|
comment:21 Changed 6 years ago by
Status: | needs_review → needs_work |
---|
Replying to jdemeyer:
Here is a proposal: - -
A good proposal!
Now I think I am happy with this. I am compiling and hoping that Sphinx is also happy. Put to needs_review to wait for that.
comment:23 Changed 6 years ago by
Branch: | u/jmantysalo/devel-manual → u/jdemeyer/devel-manual |
---|
comment:24 Changed 6 years ago by
Commit: | 8e027296608cc8998fd513ca33b35f22a5466a4f → 24e3ab9282f646164bbcfe4f45dae938d406f18f |
---|
Some decisions are arbitrary, but common convention makes the program to look polished and professional.
should be
Some decisions are arbitrary, but common convention make the program look more polished and professional.
Actually, I'd prefer
Some decisions are arbitrary, but common convention make life easier.
New commits:
24e3ab9 | Fix doc indentation
|
comment:25 follow-ups: 28 33 Changed 6 years ago by
Also, I think it is conventional to start in lower case after a colon.
comment:26 Changed 6 years ago by
Reviewers: | → Jeroen Demeyer, Martin Rubey |
---|---|
Status: | needs_review → needs_work |
comment:27 Changed 6 years ago by
Branch: | u/jdemeyer/devel-manual → u/jmantysalo/devel-manual |
---|
comment:28 Changed 6 years ago by
Commit: | 24e3ab9282f646164bbcfe4f45dae938d406f18f → c4462037eba99ccf3176844d618f76924ea906bd |
---|
Replying to mantepse:
common convention make life easier.
Good and short, changed.
Replying to mantepse:
Also, I think it is conventional to start in lower case after a colon.
I trust you on this.
(Finnish has clear rule for this: use lowercase if and only if the part before the colon refers to one clause only, and uppercase otherwise.)
New commits:
c446203 | Still more fine tuning.
|
comment:29 follow-up: 31 Changed 6 years ago by
Sorry, I missed a plural "s", it should be
Some decisions are arbitrary, but common conventions make life easier.
comment:30 Changed 6 years ago by
Commit: | c4462037eba99ccf3176844d618f76924ea906bd → 74d9bdf0f267946ecb769ffe1055398ab002c3ee |
---|
Branch pushed to git repo; I updated commit sha1. New commits:
74d9bdf | Add 's'.
|
comment:31 Changed 6 years ago by
Status: | needs_work → needs_review |
---|
Replying to mantepse:
Sorry, I missed a plural "s", it should be
Some decisions are arbitrary, but common conventions make life easier.
's' added.
comment:32 Changed 6 years ago by
Status: | needs_review → positive_review |
---|
comment:33 Changed 6 years ago by
Replying to mantepse:
Also, I think it is conventional to start in lower case after a colon.
That can be capitalized or not because the part after the colon could be a complete separate sentence. However, I feel it is better to not have it capitalized because it is being used as an explanatory clause.
comment:35 Changed 6 years ago by
Commit: | 74d9bdf0f267946ecb769ffe1055398ab002c3ee → 47df1ea05b2a9ec2d02683ca87d2ef919e18986d |
---|
Branch pushed to git repo; I updated commit sha1. Last 10 new commits:
f5a6aa3 | Minor addition.
|
0292b50 | Less duplication.
|
86e42a6 | Minor changes.
|
fe4818a | Reviewers comments.
|
e5723d8 | Additions to keywords.
|
85385f5 | Explanation of common keywords.
|
5f007b0 | Fix doc indentation
|
b1e83b1 | Still more fine tuning.
|
d6e0d40 | Add 's'.
|
47df1ea | Merge branch 'u/jmantysalo/devel-manual' of trac.sagemath.org:sage into t/21646/devel-manual
|
comment:38 Changed 6 years ago by
Commit: | 47df1ea05b2a9ec2d02683ca87d2ef919e18986d → f4818f0987b3984f77e7d7e396a8a983de19dc13 |
---|
Branch pushed to git repo; I updated commit sha1. Last 10 new commits:
e53d89b | Fix doc indentation
|
6541f18 | Still more fine tuning.
|
819a989 | Add 's'.
|
5a7fda6 | Devel manual: docstring writing
|
a53ff2a | Minor addition.
|
546dd66 | Less duplication.
|
d4254a4 | Minor changes.
|
b96d03d | Reviewers comments.
|
8297b5b | Additions to keywords.
|
f4818f0 | Rebase to 7.5beta0
|
comment:40 Changed 6 years ago by
Status: | needs_review → positive_review |
---|
comment:41 Changed 6 years ago by
Branch: | u/jmantysalo/devel-manual → f4818f0987b3984f77e7d7e396a8a983de19dc13 |
---|---|
Resolution: | → fixed |
Status: | positive_review → closed |
Hmm... There are already a "What doctests should test" -part, so at least part of this patch is duplicate.
New commits:
Devel manual: docstring writing