Opened 5 years ago

Closed 5 years ago

#17508 closed enhancement (fixed)

Reformat the developer's manual's page about docstrings

Reported by: ncohen Owned by:
Priority: major Milestone: sage-6.5
Component: documentation Keywords:
Cc: kcrisman Merged in:
Authors: Nathann Cohen Reviewers: Karl-Dieter Crisman
Report Upstream: N/A Work issues:
Branch: 8728734 (Commits) Commit: 87287342917237b80cc2505b4dd310df4e2a3f8d
Dependencies: Stopgaps:

Description

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

Change History (14)

comment:1 Changed 5 years ago by ncohen

  • Branch set to public/17508

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

comment:2 Changed 5 years ago by git

  • Commit set to 5f4d0b3d6ca5c4174711d1fa2a31a19d14bbe367

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

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

comment:3 Changed 5 years ago by ncohen

  • Cc kcrisman added
  • Status changed from new to needs_review

comment:4 Changed 5 years ago by kcrisman

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 ^^^^ 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!

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

comment:5 Changed 5 years ago by ncohen

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

comment:6 Changed 5 years ago by kcrisman

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

comment:7 Changed 5 years ago by git

  • Commit changed from 5f4d0b3d6ca5c4174711d1fa2a31a19d14bbe367 to d8ba8879560ceae60a0ccd1acdfc6aa46ef0b1a5

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

d8ba887trac #17508: Reviewer's remarks

comment:8 Changed 5 years ago by ncohen

For some reason it works today.... O_o

Anyway. Good :-P

Nathann

comment:9 Changed 5 years ago by kcrisman

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.

comment:10 Changed 5 years ago by git

  • Commit changed from d8ba8879560ceae60a0ccd1acdfc6aa46ef0b1a5 to 87287342917237b80cc2505b4dd310df4e2a3f8d

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

8728734trac #17508: Reviewer's remarks 2

comment:11 Changed 5 years ago by ncohen

Heeeeeeere it is !

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

Nathann

comment:12 Changed 5 years ago by kcrisman

  • Reviewers set to Karl-Dieter Crisman
  • Status changed from needs_review to positive_review

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.

comment:13 Changed 5 years ago by ncohen

Thanks for the review !

Nathann

comment:14 Changed 5 years ago by vbraun

  • Branch changed from public/17508 to 87287342917237b80cc2505b4dd310df4e2a3f8d
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.