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

Devel manual about docstrings #21646

Closed
jm58660 mannequin opened this issue Oct 5, 2016 · 50 comments
Closed

Devel manual about docstrings #21646

jm58660 mannequin opened this issue Oct 5, 2016 · 50 comments

Comments

@jm58660
Copy link
Mannequin

jm58660 mannequin commented Oct 5, 2016

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?

CC: @tscrim @fchapoton

Component: documentation

Author: Jori Mäntysalo

Branch/Commit: f4818f0

Reviewer: Jeroen Demeyer, Martin Rubey

Issue created by migration from https://trac.sagemath.org/ticket/21646

@jm58660 jm58660 mannequin added this to the sage-7.4 milestone Oct 5, 2016
@jm58660 jm58660 mannequin added c: documentation labels Oct 5, 2016
@jm58660
Copy link
Mannequin Author

jm58660 mannequin commented Oct 5, 2016

Branch: u/jmantysalo/devel-manual

@jm58660
Copy link
Mannequin Author

jm58660 mannequin commented Oct 5, 2016

Commit: ff65397

@jm58660
Copy link
Mannequin Author

jm58660 mannequin commented Oct 5, 2016

comment:2

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

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Oct 15, 2016

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

cb7c34cMinor addition.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Oct 15, 2016

Changed commit from ff65397 to cb7c34c

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Oct 15, 2016

Changed commit from cb7c34c to 49d57c8

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Oct 15, 2016

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

49d57c8Less duplication.

@jm58660
Copy link
Mannequin Author

jm58660 mannequin commented Oct 15, 2016

comment:5

No comments got.

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

@jm58660 jm58660 mannequin modified the milestones: sage-7.4, sage-7.5 Oct 15, 2016
@jm58660 jm58660 mannequin added the s: needs review label Oct 15, 2016
@jdemeyer
Copy link

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

  2. I don't like advice of the form Do not use ... Be positive, say only what should be used.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Oct 15, 2016

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

f61ed30Minor changes.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Oct 15, 2016

Changed commit from 49d57c8 to f61ed30

@jm58660
Copy link
Mannequin Author

jm58660 mannequin commented Oct 15, 2016

comment:8

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?

@jm58660 jm58660 mannequin added s: needs review and removed s: needs work labels Oct 15, 2016
@jhpalmieri
Copy link
Member

comment:10

There is still one more tab. Could you also make this grammatical fix:

diff --git a/src/doc/en/developer/coding_basics.rst b/src/doc/en/developer/coding_basics.rst
index 60e49ec..d65e6d7 100644
--- a/src/doc/en/developer/coding_basics.rst
+++ b/src/doc/en/developer/coding_basics.rst
@@ -283,7 +283,7 @@ information. You can use the existing functions of Sage as templates.
    They should have good coverage of the functionality in question.
 
 -  A **SEEALSO** block (highly recommended) with links to related parts of
-   Sage. This helps users find the features that interests them and discover
+   Sage. This helps users find the features that interest them and discover
    the new ones. ::
 
        .. SEEALSO::

@jdemeyer
Copy link

comment:11

Replying to @jm58660:

How about this "instead of" -formulation?

Why not simply say "Use X"?

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Oct 17, 2016

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

1b06af9Reviewers comments.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Oct 17, 2016

Changed commit from f61ed30 to 1b06af9

@jm58660
Copy link
Mannequin Author

jm58660 mannequin commented Oct 17, 2016

comment:13

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.

@jdemeyer
Copy link

comment:14

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.

  • ...

@jm58660
Copy link
Mannequin Author

jm58660 mannequin commented Oct 17, 2016

comment:15

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?

@mantepse
Copy link
Collaborator

comment:29

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

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

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Oct 18, 2016

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

74d9bdfAdd 's'.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Oct 18, 2016

Changed commit from c446203 to 74d9bdf

@jm58660
Copy link
Mannequin Author

jm58660 mannequin commented Oct 18, 2016

comment:31

Replying to @mantepse:

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

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

's' added.

@tscrim
Copy link
Collaborator

tscrim commented Oct 18, 2016

comment:33

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.

@vbraun
Copy link
Member

vbraun commented Oct 19, 2016

comment:34

merge conflict

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Oct 20, 2016

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

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Oct 20, 2016

Changed commit from 74d9bdf to 47df1ea

@jm58660
Copy link
Mannequin Author

jm58660 mannequin commented Oct 20, 2016

comment:36

New try.

@jm58660 jm58660 mannequin added s: needs review and removed s: needs work labels Oct 20, 2016
@jdemeyer
Copy link

comment:37

Conflicts with 7.5.beta0

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Oct 21, 2016

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

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Oct 21, 2016

Changed commit from 47df1ea to f4818f0

@jm58660
Copy link
Mannequin Author

jm58660 mannequin commented Oct 21, 2016

comment:39

Another try.

@vbraun
Copy link
Member

vbraun commented Oct 29, 2016

Changed branch from u/jmantysalo/devel-manual to f4818f0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants