Opened 10 years ago

Closed 10 years ago

#6840 closed defect (fixed)

[with patch, positive review] Fix documentation for Sage Notebook

Reported by: timdumol Owned by: boothby
Priority: minor Milestone: sage-4.1.2
Component: notebook Keywords: notebook documentation
Cc: Merged in: Sage 4.1.2.alpha0
Authors: Tim Dumol, Mitesh Patel Reviewers: Minh Van Nguyen, Mitesh Patel
Report Upstream: Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Description

Change documentation of the notebook to a uniform format and complete notebook doctest/documentation coverage.

  • Some docstrings are improperly formatted.
  • The formatting of docstrings is inconsistent, e.g.:

Some have:

Output: footype

Description

While others have:

Output:

- footype -- description

etc.

Attachments (8)

trac_6840-notebook-documentation.patch (12.8 KB) - added by timdumol 10 years ago.
Fixes docstrings added by patch #6568 in notebook.py and worksheet.py. Depends on #6568
trac_6840-notebook-documentation-v2.patch (112.6 KB) - added by timdumol 10 years ago.
Merged changes from #5360. Apply only this file.
trac_6840-notebook-documentation.v2.patch (2 bytes) - added by timdumol 10 years ago.
OBSOLETE.
trac_6840-notebook-documentation-v3.patch (155.9 KB) - added by mpatel 10 years ago.
Version 3. Apply only this patch.
trac_6840-notebook-documentation-v4.patch (167.1 KB) - added by timdumol 10 years ago.
Version 4. Brought up cell.py coverage to almost 100%, except for 2 functions.
trac_6840-notebook-documentation-v5.patch (167.3 KB) - added by timdumol 10 years ago.
Fixed documentation warnings and doctest failure. Apply this patch only.
trac_6840-notebook-documentation-v6.patch (173.0 KB) - added by mpatel 10 years ago.
Miscellaneous tiny changes (see comment). Apply only this patch.
trac_6840-reviewer.patch (26.2 KB) - added by mvngu 10 years ago.
apply on top of previous patch

Download all attachments as: .zip

Change History (25)

Changed 10 years ago by timdumol

Fixes docstrings added by patch #6568 in notebook.py and worksheet.py. Depends on #6568

comment:1 Changed 10 years ago by mpatel

I think there is some "overlap" here with version 2 of the patch at #5360. But that patch makes only a few changes to notebook.py. To avoid merge conflicts, should I put them in a separate patch or bring them here?

comment:2 Changed 10 years ago by timdumol

If you don't mind, I merged the changes from #5360 to save you the trouble.

comment:3 Changed 10 years ago by mpatel

Thanks! #5360 is now marked for closure.

Changed 10 years ago by timdumol

Merged changes from #5360. Apply only this file.

Changed 10 years ago by timdumol

OBSOLETE.

comment:4 Changed 10 years ago by mpatel

I think the patch somehow got doubled in size during the merger.

comment:5 Changed 10 years ago by timdumol

Yes, it got messed up the first times around, but it seems to be the right size: trac_6840-notebook-documentation-v2.patch (112.6 KB).

comment:6 Changed 10 years ago by mpatel

Version 3 adds lots of small changes to introspect.py, misc.py, docHTMLProcessor.py, interact.py, notebook.py, template.py, and support.py.

Topics: missing docstrings, directives, blank lines, bullet points, headings; other inconsistencies; Sphinx warnings; etc. The doctests pass, apart the known problem with randomly ordered tests in worksheet.py.

I apologize for stepping on any toes. There may still be other problems, besides those I just introduced. Prepping server/ for the reference manual is tedious, but the docstrings do appear to render somewhat more nicely now, especially those in interact.py.

Please feel free to make more changes. Or we can just put this up for review and fix the remaining quirks as we find them.

Changed 10 years ago by mpatel

Version 3. Apply only this patch.

Changed 10 years ago by timdumol

Version 4. Brought up cell.py coverage to almost 100%, except for 2 functions.

comment:7 Changed 10 years ago by timdumol

  • Summary changed from Fix documentation for Sage Notebook to [with patch, needs review]Fix documentation for Sage Notebook

Version 4 adds docstrings and doctests for cell.py except for 2 functions: one of which is a hack (doc_html), and the other I have no idea how to test (stop_interacting).

I'll put this up for review, and we can work on the rest of the documentation another time (unless you want to add some more now?).

comment:8 Changed 10 years ago by mvngu

  • Summary changed from [with patch, needs review]Fix documentation for Sage Notebook to [with patch, needs work]Fix documentation for Sage Notebook

The patch trac_6840-notebook-documentation-v4.patch applies OK against 4.1.1, but with one fuzz:

[mvngu@sage sage-main]$ hg qimport http://trac.sagemath.org/sage_trac/raw-attachment/ticket/6840/trac_6840-notebook-documentation-v4.patch && hg qpush
adding trac_6840-notebook-documentation-v4.patch to series file
applying trac_6840-notebook-documentation-v4.patch
patching file sage/server/notebook/config.py
Hunk #1 succeeded at 1 with fuzz 1 (offset -1 lines).
Now at: trac_6840-notebook-documentation-v4.patch

Building the reference manual results in about 10 warnings:

WARNING: /scratch/mvngu/release/sage-4.1.1/local/lib/python2.6/site-packages/sage/server/notebook/cell.py:docstring of sage.server.notebook.cell.Cell.html:8: (WARNING/2) Inline literal start-string without end-string.
WARNING: /scratch/mvngu/release/sage-4.1.1/local/lib/python2.6/site-packages/sage/server/notebook/cell.py:docstring of sage.server.notebook.cell.Cell.html_out:6: (WARNING/2) Bullet list ends without a blank line; unexpected unindent.
WARNING: /scratch/mvngu/release/sage-4.1.1/local/lib/python2.6/site-packages/sage/server/notebook/cell.py:docstring of sage.server.notebook.cell.Cell.is_interacting:8: (WARNING/2) Block quote ends without a blank line; unexpected unindent.
WARNING: /scratch/mvngu/release/sage-4.1.1/local/lib/python2.6/site-packages/sage/server/notebook/cell.py:docstring of sage.server.notebook.cell.Cell.output_text:10: (WARNING/2) Bullet list ends without a blank line; unexpected unindent.
WARNING: /scratch/mvngu/release/sage-4.1.1/local/lib/python2.6/site-packages/sage/server/notebook/cell.py:docstring of sage.server.notebook.cell.Cell.parse_html:15: (WARNING/2) Block quote ends without a blank line; unexpected unindent.
WARNING: /scratch/mvngu/release/sage-4.1.1/local/lib/python2.6/site-packages/sage/server/notebook/cell.py:docstring of sage.server.notebook.cell.ComputeCell.html:8: (WARNING/2) Inline literal start-string without end-string.
WARNING: /scratch/mvngu/release/sage-4.1.1/local/lib/python2.6/site-packages/sage/server/notebook/cell.py:docstring of sage.server.notebook.cell.ComputeCell.html_out:6: (WARNING/2) Bullet list ends without a blank line; unexpected unindent.
WARNING: /scratch/mvngu/release/sage-4.1.1/local/lib/python2.6/site-packages/sage/server/notebook/cell.py:docstring of sage.server.notebook.cell.ComputeCell.is_interacting:8: (WARNING/2) Block quote ends without a blank line; unexpected unindent.
WARNING: /scratch/mvngu/release/sage-4.1.1/local/lib/python2.6/site-packages/sage/server/notebook/cell.py:docstring of sage.server.notebook.cell.ComputeCell.output_text:10: (WARNING/2) Bullet list ends without a blank line; unexpected unindent.
WARNING: /scratch/mvngu/release/sage-4.1.1/local/lib/python2.6/site-packages/sage/server/notebook/cell.py:docstring of sage.server.notebook.cell.ComputeCell.parse_html:15: (WARNING/2) Block quote ends without a blank line; unexpected unindent.

The test suite reports one doctest failure:

sage -t -long devel/sage-main/sage/server/notebook/cell.py
**********************************************************************
File "/scratch/mvngu/release/sage-4.1.1/devel/sage-main/sage/server/notebook/cell.py", line 1598:
    sage: C.introspect_html()
Expected:
    '<div class="docstring">\n    \n  <p><span class="math">foobar</span></p>\n\n\n</div>'
Got:
    '<div class="docstring">\n    \n  \n  <p><span class="math">foobar</span></p>\n\n\n</div>'
**********************************************************************
1 items had failures:
   1 of  13 in __main__.example_71
***Test Failed*** 1 failures.
For whitespace errors, see the file /scratch/mvngu/release/sage-4.1.1/tmp/.doctest_cell.py
	 [26.4 s]

I'm marking this as "needs work", since at least the doctest failure must be resolved. The warnings from building the reference manual can be addressed via another ticket. However, it would be nice if both of these issues could be fixed here.

comment:9 Changed 10 years ago by timdumol

  • Summary changed from [with patch, needs work]Fix documentation for Sage Notebook to [with patch, needs review]Fix documentation for Sage Notebook

I've fixed the warnings and the doctest failure.

Changed 10 years ago by timdumol

Fixed documentation warnings and doctest failure. Apply this patch only.

Changed 10 years ago by mpatel

Miscellaneous tiny changes (see comment). Apply only this patch.

comment:10 Changed 10 years ago by mpatel

  • Authors set to Tim Dumol, Mitesh Patel
  • Summary changed from [with patch, needs review]Fix documentation for Sage Notebook to [with patch, needs review] Fix documentation for Sage Notebook

v6 changes:

  • Fixed INPUT:: in template.py.
  • Removed the space immediately after :meth: in two places in cell.py.
  • Removed config.py from notebook.rst, since there's no significant documentation yet on notebook key bindings.

That's impressive coverage for cell.py! It seems that Cell.stop_interacting() isn't even called in the Sage library.

comment:11 Changed 10 years ago by mpatel

One way to placate Sphinx about the unused source file doc/en/reference/sage/server/notebook/config.rst: Delete doc/output/*/en/reference/ and rebuild from scratch.

comment:12 Changed 10 years ago by mvngu

The patch trac_6840-notebook-documentation-v6.patch applies OK, but with fuzz:

[mvngu@sage sage-main]$ hg qimport http://trac.sagemath.org/sage_trac/raw-attachment/ticket/6840/trac_6840-notebook-documentation-v6.patch && hg qpush
adding trac_6840-notebook-documentation-v6.patch to series file
applying trac_6840-notebook-documentation-v6.patch
patching file sage/server/notebook/config.py
Hunk #1 succeeded at 1 with fuzz 1 (offset -1 lines).
Now at: trac_6840-notebook-documentation-v6.patch

Don't worry too much about the fuzz. I have attached a reviewer patch that fixes typos introduced by the previous patch. If you're OK with that reviewer patch, then the ticket gets a positive review and patches should be merged in this order:

  1. trac_6840-notebook-documentation-v6.patch
  2. trac_6840-reviewer.patch

comment:13 Changed 10 years ago by mpatel

Thanks very much for fixing the typos, including AttributeErrro.

Is it alright if we treat the "PLANNING" section near the top of interact.py as "internal" documentation, at least for now? I haven't checked that it's current and Sphinx complains copiously about it. Either way, just let me know, and I'll attach v2 of the reviewer patch, since I found at least one more minor change to make (2D to 2-D).

By the way, is there a reST alternative to double back-quotes that puts the enclosed text in a fixed-width font but does not change its background color?

comment:14 follow-up: Changed 10 years ago by mpatel

While I'm here I should ask whether we should put both side-effects, including possible exceptions, and return values in OUTPUT sections. Mostly, I've mentioned just the return values in OUTPUT. On occasion, I've noted significant effects in the blurb at the top of a docstring.

Changed 10 years ago by mvngu

apply on top of previous patch

comment:15 in reply to: ↑ 14 Changed 10 years ago by mvngu

Replying to mpatel:

Is it alright if we treat the "PLANNING" section near the top of interact.py as "internal" documentation, at least for now? I haven't checked that it's current and Sphinx complains copiously about it.

New reviewer patch attached. Please use that one. That should get rid of the complaints by Sphinx.


Either way, just let me know, and I'll attach v2 of the reviewer patch, since I found at least one more minor change to make (2D to 2-D).

The updated reviewer patch also addresses this issue.


By the way, is there a reST alternative to double back-quotes that puts the enclosed text in a fixed-width font but does not change its background color?

I don't know.


Replying to mpatel:

While I'm here I should ask whether we should put both side-effects, including possible exceptions, and return values in OUTPUT sections. Mostly, I've mentioned just the return values in OUTPUT. On occasion, I've noted significant effects in the blurb at the top of a docstring.

That's a good idea. Document what works and what doesn't. I usually have a "TESTS:" section for a method/function/class where I document exceptions that could be raised when using that method/function/class.

comment:16 Changed 10 years ago by mpatel

  • Authors changed from Tim Dumol, Mitesh Patel to Tim Dumol, Minh Van Nguyen, Mitesh Patel
  • Reviewers set to Minh Van Nguyen, Mitesh Patel
  • Summary changed from [with patch, needs review] Fix documentation for Sage Notebook to [with patch, positive review] Fix documentation for Sage Notebook

comment:17 Changed 10 years ago by mvngu

  • Authors changed from Tim Dumol, Minh Van Nguyen, Mitesh Patel to Tim Dumol, Mitesh Patel
  • Merged in set to Sage 4.1.2.alpha0
  • Resolution set to fixed
  • Status changed from new to closed

Merged patches in this order:

  1. trac_6840-notebook-documentation-v6.patch
  2. trac_6840-reviewer.patch

I take reviewer credit, but not authorship credit :-)

Note: See TracTickets for help on using tickets.