Opened 5 years ago

Closed 5 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:

Status badges

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 5 years ago by jmantysalo

  • Branch set to u/jmantysalo/devel-manual

comment:2 Changed 5 years ago by jmantysalo

  • Commit set to ff653973cf1ec43e238a612ca81e97f3e2995f10

Hmm... There are already a "What doctests should test" -part, so at least part of this patch is duplicate.


New commits:

ff65397Devel manual: docstring writing

comment:3 Changed 5 years ago by git

  • Commit changed from ff653973cf1ec43e238a612ca81e97f3e2995f10 to cb7c34c805dd3c20c25b549d95d23b71ae0140b3

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

cb7c34cMinor addition.

comment:4 Changed 5 years ago by git

  • Commit changed from cb7c34c805dd3c20c25b549d95d23b71ae0140b3 to 49d57c85304078ae0a7fe49c3a2fd7d6e966f8f8

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

49d57c8Less duplication.

comment:5 Changed 5 years ago by jmantysalo

  • Milestone changed from sage-7.4 to sage-7.5
  • Status changed from new to needs_review

No comments got.

Anyways, this now encourages slightly more to use TESTS block. Also I documented some arbitrary decisions made.

Last edited 5 years ago by jmantysalo (previous) (diff)

comment:6 follow-up: Changed 5 years ago by jdemeyer

  • Status changed from needs_review to needs_work
  1. The example of a TESTS block is badly indented (it was already badly indented by 3 spaces only).
  1. I don't like advice of the form Do not use ... Be positive, say only what should be used.

comment:7 Changed 5 years ago by git

  • Commit changed from 49d57c85304078ae0a7fe49c3a2fd7d6e966f8f8 to f61ed30c2e39c257d96d20696ce5fb00788d0f6c

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

f61ed30Minor changes.

comment:8 in reply to: ↑ 6 ; follow-up: Changed 5 years ago by jmantysalo

Replying to jdemeyer:

  1. The example of a TESTS block is badly indented (it was already badly indented by 3 spaces only).

There were tabs. Corrected.

  1. 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 5 years ago by jmantysalo

  • Status changed from needs_work to needs_review

comment:10 Changed 5 years ago by jhpalmieri

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. 
    283283   They should have good coverage of the functionality in question.
    284284
    285285-  A **SEEALSO** block (highly recommended) with links to related parts of
    286    Sage. This helps users find the features that interests them and discover
     286   Sage. This helps users find the features that interest them and discover
    287287   the new ones. ::
    288288
    289289       .. SEEALSO::

comment:11 in reply to: ↑ 8 ; follow-up: Changed 5 years ago by jdemeyer

Replying to jmantysalo:

How about this "instead of" -formulation?

Why not simply say "Use X"?

comment:12 Changed 5 years ago by git

  • Commit changed from f61ed30c2e39c257d96d20696ce5fb00788d0f6c to 1b06af9127725ffc862acb9dc134be0e6768eaea

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

1b06af9Reviewers comments.

comment:13 in reply to: ↑ 11 Changed 5 years ago by jmantysalo

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: Changed 5 years ago by jdemeyer

I was thinking that you could document a few standard keywords in the same way:

  • algorithm: choose between various implementation (with None meaning a sensible default, which could depend on installed optional packages).
  • proof: is this is True, require a mathematically proven computation. With False, 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 with check=True, it should also work with check=False).
  • coerce: convert the input parameters to a suitable parent. This is typically used in constructors. You can call a method with coerce=False to skip some checks if the parent is known to be correct.
  • ...

comment:15 in reply to: ↑ 14 ; follow-up: Changed 5 years ago by jmantysalo

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 5 years ago by git

  • Commit changed from 1b06af9127725ffc862acb9dc134be0e6768eaea to edb1e67b15e78ae792701a190a0b33756fdbfa1f

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

edb1e67Additions to keywords.

comment:17 in reply to: ↑ 15 ; follow-up: Changed 5 years ago by 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.

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 in reply to: ↑ 17 Changed 5 years ago by jmantysalo

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: Changed 5 years ago by jdemeyer

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 5 years ago by git

  • Commit changed from edb1e67b15e78ae792701a190a0b33756fdbfa1f to 8e027296608cc8998fd513ca33b35f22a5466a4f

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

8e02729Explanation of common keywords.

comment:21 in reply to: ↑ 19 Changed 5 years ago by jmantysalo

  • Status changed from needs_review to 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:22 Changed 5 years ago by jmantysalo

  • Status changed from needs_work to needs_review

Compiled.

comment:23 Changed 5 years ago by jdemeyer

  • Branch changed from u/jmantysalo/devel-manual to u/jdemeyer/devel-manual

comment:24 Changed 5 years ago by mantepse

  • Commit changed from 8e027296608cc8998fd513ca33b35f22a5466a4f to 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:

24e3ab9Fix doc indentation

comment:25 follow-ups: Changed 5 years ago by mantepse

Also, I think it is conventional to start in lower case after a colon.

comment:26 Changed 5 years ago by jdemeyer

  • Reviewers set to Jeroen Demeyer, Martin Rubey
  • Status changed from needs_review to needs_work

comment:27 Changed 5 years ago by jmantysalo

  • Branch changed from u/jdemeyer/devel-manual to u/jmantysalo/devel-manual

comment:28 in reply to: ↑ 25 Changed 5 years ago by jmantysalo

  • Commit changed from 24e3ab9282f646164bbcfe4f45dae938d406f18f to 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:

c446203Still more fine tuning.

comment:29 follow-up: Changed 5 years ago by mantepse

Sorry, I missed a plural "s", it should be

Some decisions are arbitrary, but common conventions make life easier.

comment:30 Changed 5 years ago by git

  • Commit changed from c4462037eba99ccf3176844d618f76924ea906bd to 74d9bdf0f267946ecb769ffe1055398ab002c3ee

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

74d9bdfAdd 's'.

comment:31 in reply to: ↑ 29 Changed 5 years ago by jmantysalo

  • Status changed from needs_work to 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 5 years ago by jdemeyer

  • Status changed from needs_review to positive_review

comment:33 in reply to: ↑ 25 Changed 5 years ago by tscrim

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:34 Changed 5 years ago by vbraun

  • Status changed from positive_review to needs_work

merge conflict

comment:35 Changed 5 years ago by git

  • Commit changed from 74d9bdf0f267946ecb769ffe1055398ab002c3ee to 47df1ea05b2a9ec2d02683ca87d2ef919e18986d

Branch pushed to git repo; I updated commit sha1. Last 10 new commits:

f5a6aa3Minor addition.
0292b50Less duplication.
86e42a6Minor changes.
fe4818aReviewers comments.
e5723d8Additions to keywords.
85385f5Explanation of common keywords.
5f007b0Fix doc indentation
b1e83b1Still more fine tuning.
d6e0d40Add 's'.
47df1eaMerge branch 'u/jmantysalo/devel-manual' of trac.sagemath.org:sage into t/21646/devel-manual

comment:36 Changed 5 years ago by jmantysalo

  • Status changed from needs_work to needs_review

New try.

comment:37 Changed 5 years ago by jdemeyer

  • Status changed from needs_review to needs_work

Conflicts with 7.5.beta0

comment:38 Changed 5 years ago by git

  • Commit changed from 47df1ea05b2a9ec2d02683ca87d2ef919e18986d to f4818f0987b3984f77e7d7e396a8a983de19dc13

Branch pushed to git repo; I updated commit sha1. Last 10 new commits:

e53d89bFix doc indentation
6541f18Still more fine tuning.
819a989Add 's'.
5a7fda6Devel manual: docstring writing
a53ff2aMinor addition.
546dd66Less duplication.
d4254a4Minor changes.
b96d03dReviewers comments.
8297b5bAdditions to keywords.
f4818f0Rebase to 7.5beta0

comment:39 Changed 5 years ago by jmantysalo

  • Status changed from needs_work to needs_review

Another try.

comment:40 Changed 5 years ago by jdemeyer

  • Status changed from needs_review to positive_review

comment:41 Changed 5 years ago by vbraun

  • Branch changed from u/jmantysalo/devel-manual to f4818f0987b3984f77e7d7e396a8a983de19dc13
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.