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

Reformat the developer's manual's page about docstrings #17508

Closed
nathanncohen mannequin opened this issue Dec 15, 2014 · 18 comments
Closed

Reformat the developer's manual's page about docstrings #17508

nathanncohen mannequin opened this issue Dec 15, 2014 · 18 comments

Comments

@nathanncohen
Copy link
Mannequin

nathanncohen mannequin commented Dec 15, 2014

The page of the developer's manual that explains how the doc of a function should be written is a very important one. It can be obtained at this address:

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

Right now, however, it is a bit hard to read for the text is a bit "flat": it is a long enumeration of blocks, and it is hard to find the one we can be looking for.

At first I only wanted to rewrite in bold the name of each block, i.e. note, warning, examples, ...

I ended up rephrasing several paragraphs in order to make them more direct (lots of information there, let us be concise). I also removed several comments saying "a .. NOTE:: block should begin with ".. NOTE::" as they were followed by an example. Or sentences like "patches will not be accepted unless they contain a X block" given that we already say at the beginning if a block is optional of not.

I hope that it will be easier to read this way.

Nathann

CC: @kcrisman

Component: documentation

Author: Nathann Cohen

Branch/Commit: 8728734

Reviewer: Karl-Dieter Crisman

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

@nathanncohen nathanncohen mannequin added this to the sage-6.5 milestone Dec 15, 2014
@nathanncohen
Copy link
Mannequin Author

nathanncohen mannequin commented Dec 15, 2014

Branch: public/17508

@nathanncohen
Copy link
Mannequin Author

nathanncohen mannequin commented Dec 15, 2014

comment:1

This branch is in conflict with #17498: once one of the two will be reviewer I will update the other.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Dec 15, 2014

Commit: 5f4d0b3

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Dec 15, 2014

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

5f4d0b3trac #17508: Reformat the 'docstring' section of the developer's manual

@nathanncohen nathanncohen mannequin added the s: needs review label Dec 16, 2014
@kcrisman
Copy link
Member

comment:4

I like this very much, in general.

Minor comments.

  • 'expected output' is better than only 'returns', because some functions return None but still have output (printing, showing a graphic, whatever).
  • It would be amusing to use the seealso directive to point to the hyperlinks reference. But perhaps too self-referential :)
  • Probably the comment in the note should still say something (less verbose is fine) about using spaces, not tabs; the current text could be misleading to someone who would then get roughed up on Trac for it.
  • instead of "it contains the tests" use "containing tests"
  • For some reason the tests info is immediately after the references info, while seealso has a nice space after examples. Maybe a missing line? (I'm reviewing this before looking at the patch.)
  • I did not know about the ..automethod and plan to use it in the future!
  • 'Note the indentation' should have a period, probably, since the double colon likely disappears or something; it looks awkward (even in the current doc) without it.

Edit after looking at code:

  • I would make
Template
--------

be at the next lower level of header, I'm not sure whether there are any such in the file - maybe use <sup>^</sup>^ for that?

  • Similarly, I'd add another subsubheader before the info about private functions, as that is not about the template. Maybe
Private Functions
^^^^^^^^^^^^^^^^^

or something like that.

Anyway, as I said, this is a nice improvement, thank you!

@nathanncohen
Copy link
Mannequin Author

nathanncohen mannequin commented Dec 16, 2014

comment:5

Helloo ! Thanks for the review.

I made the modifications you asked but right now it is only "on my computer". I can't seem to access internet without proxy these days, and that means no ssh connections --> cannot use git. I hope that I will find a way tomorrow :-/

  • 'expected output' is better than only 'returns', because some functions return None but still have output (printing, showing a graphic, whatever).

Done

  • It would be amusing to use the seealso directive to point to the hyperlinks reference. But perhaps too self-referential :)

I thought of illustrating "warnings" with a warning, "notes" with a note, then decided against it as the big colored blocks it produced were "stealing the attention".

  • Probably the comment in the note should still say something (less verbose is fine) about using spaces, not tabs; the current text could be misleading to someone who would then get roughed up on Trac for it.

I added some words about that.

  • instead of "it contains the tests" use "containing tests"

Done.

  • For some reason the tests info is immediately after the references info, while seealso has a nice space after examples. Maybe a missing line? (I'm reviewing this before looking at the patch.)

No, just Sphinx not liking to have a new entry "A TESTS block [...]" right after the end of an enumeration. I turned it into normal text, it is not that bad either.

  • I did not know about the ..automethod and plan to use it in the future!

Yep, it's a cool thing.

  • 'Note the indentation' should have a period, probably, since the double colon likely disappears or something; it looks awkward (even in the current doc) without it.

Done

Edit after looking at code:

  • I would make
Template
--------

be at the next lower level of header

Done, looks good.

  • Similarly, I'd add another subsubheader before the info about private functions

Done.

The git branch will be updated as soon as I can get a real connection somewhere T_T

Nathann

@kcrisman
Copy link
Member

comment:6

Awesome, and please no rush to go to a McDonald's or someplace with free presumably non-proxied wifi ;-)

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Dec 17, 2014

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

d8ba887trac #17508: Reviewer's remarks

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Dec 17, 2014

Changed commit from 5f4d0b3 to d8ba887

@nathanncohen
Copy link
Mannequin Author

nathanncohen mannequin commented Dec 17, 2014

comment:8

For some reason it works today.... O_o

Anyway. Good :-P

Nathann

@kcrisman
Copy link
Member

comment:9

Trivial final comments:

  • In the template we have
    REFERENCES:

    .. [BCDT] Breuil, Conrad, Diamond, Taylor,
       "Modularity ...."

but in the text example you have

      .. [SC] Conventions for coding in sage.
        http://www.sagemath.org/doc/developer/conventions.html.

Note the one-space discrepancy. Does it matter? I don't know.

  • While Never use tabulations. may be technically correct, I think that most people coming to this will have to look up whether that has anything to do with the tab key, like I did. Usually in (American?) English "tabulation" refers to some kind of tallying or scoring procedure. Maybe Never use tabs (tab characters). or something?

This is really nice, it will help a lot.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Dec 17, 2014

Changed commit from d8ba887 to 8728734

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Dec 17, 2014

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

8728734trac #17508: Reviewer's remarks 2

@nathanncohen
Copy link
Mannequin Author

nathanncohen mannequin commented Dec 17, 2014

comment:11

Heeeeeeere it is !

The space does not change anything, so I simplified to the most satisfying text format :-P

Nathann

@kcrisman
Copy link
Member

Reviewer: Karl-Dieter Crisman

@kcrisman
Copy link
Member

comment:12

The space does not change anything, so I simplified to the most satisfying text format :-P

Awesome. As a reward you now get to rebase #17498 against this.

@nathanncohen
Copy link
Mannequin Author

nathanncohen mannequin commented Dec 17, 2014

comment:13

Thanks for the review !

Nathann

@vbraun
Copy link
Member

vbraun commented Dec 18, 2014

Changed branch from public/17508 to 8728734

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

2 participants