Opened 12 years ago

Last modified 5 weeks ago

#10080 needs_work enhancement

Fix documentation look in symbolic/expression.pyx

Reported by: kcrisman Owned by: mvngu
Priority: minor Milestone:
Component: documentation Keywords: sd35.5
Cc: mvngu, jpflori Merged in:
Authors: Kenneth Smith, Kannappan Sampath Reviewers: Travis Scrimshaw, Kannappan Sampath
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Status badges

Description (last modified by knsam)

In this file, there are a lot of places where, in the html documentation, one sees TEST areas that aren't highlighted, methods like simplify_log referred to but not hyperlinked via :meth: (in one case, somehow it is only meth: by mistake), and so forth. I won't be more specific because it's throughout the file - you just have to look. For instance, in a number of places there is something like

TESTS::
     sage: asf;dlkj

but since there isn't an intervening empty line this is typeset wrong.


Apply : trac_10080_docstringsfix1.patch, trac_10080_docstringsfix2.patch, attachment: trac_10080_docstringsfix3.patch]

Attachments (5)

trac_10080_symbolic_expression.patch (13.9 KB) - added by ksmith 11 years ago.
trac_10080_docstrings_edit.patch (57.9 KB) - added by knsam 10 years ago.
The Patch!
trac_10080_docstringsfix1.patch (66.7 KB) - added by knsam 10 years ago.
First Part.
trac_10080_docstringsfix2.patch (2.5 KB) - added by knsam 10 years ago.
Second and the last part
trac_10080_docstringsfix3.patch (991 bytes) - added by knsam 10 years ago.
Not quite! Perhaps, the last!

Download all attachments as: .zip

Change History (27)

comment:1 Changed 12 years ago by kcrisman

Description: modified (diff)

comment:2 Changed 11 years ago by ksmith

Authors: Kenneth Smith
Keywords: sd35.5 added
Status: newneeds_review

I made a patch that fixed all but two of the errors. I'm still getting:

docstring of sage.symbolic.expression:10: (ERROR/3) Unexpected indentation.

and

docstring of sage.symbolic.expression:12: (WARNING/2) Block quote ends without a blank line; unexpected unindent.

Changed 11 years ago by ksmith

comment:3 Changed 11 years ago by ksmith

Status: needs_reviewneeds_work

comment:4 Changed 10 years ago by knsam

The docstrings have been edited to confirm to our standards. (Personally, it helped me learn quite a bit about formatting the documentation and using an editor properly.)

Changed 10 years ago by knsam

The Patch!

comment:5 Changed 10 years ago by knsam

Status: needs_workneeds_review

comment:6 Changed 10 years ago by tscrim

Hey,

Many of the things that were there before were correct (see the conventions page). However the biggest thing to strive for (in a file) is consistency. For example, every occurrence of True should be typeset as ``True`` and every trac reference should be of the form :trac:`10080`. (Of course there are some exceptions to this "rule", but the reasoning should be clear and self-evident.) However this does clean up a fair amount and is a good first attempt.

Also, is your patch suppose to be the only patch applied or on top of Kenneth's patch?

Thanks,
Travis

comment:7 Changed 10 years ago by tscrim

Status: needs_reviewneeds_work

comment:8 in reply to:  6 ; Changed 10 years ago by knsam

Replying to tscrim: Hi!

Hey,

Many of the things that were there before were correct (see the conventions page). However the biggest thing to strive for (in a file) is consistency. For example, every occurrence of True should be typeset as ``True``

I might be mistaken, but I went through them once. I would be glad to have some pointers for a start about the old style being the right one and "True".

and every trac reference should be of the form :trac:`10080`. (Of course there are some exceptions to this "rule", but the reasoning should be clear and self-evident.)

OK Will do!

However this does clean up a fair amount and is a good first attempt.

Also, is your patch suppose to be the only patch applied or on top of Kenneth's patch?

Mine is the supposed to be the only patch you apply.

Thanks,
Travis

comment:9 in reply to:  8 ; Changed 10 years ago by tscrim

Description: modified (diff)
Reviewers: Travis Scrimshaw

Replying to knsam:

Replying to tscrim:

Many of the things that were there before were correct (see the conventions page). However the biggest thing to strive for (in a file) is consistency. For example, every occurrence of True should be typeset as ``True``

I might be mistaken, but I went through them once. I would be glad to have some pointers for a start about the old style being the right one and "True".

For example, the INPUT: and OUTPUT: blocks should be:

INPUT:

- ``arg_name`` -- Description of argument

- ``second_arg`` -- Another description

OUTPUT:

Describing the output

Also, for multi-line doctests, it is just ..., not ....: (don't ask me why), I prefer EXAMPLES: (with the s no matter if it is 1 or many), and capitalized .. SEEALSO:: and such blocks. A good check is to run sage -docbuild reference html (after rebuilding sage via sage -b) and look at the output in .../sage-branch/doc/output/html/... in your favorite web-browser (Firefox had some issues last I knew, this might have been fixed...).

Mine is the supposed to be the only patch you apply.

For patchbot:

Apply only: trac_10080_docstrings_edit.patch

comment:10 in reply to:  9 ; Changed 10 years ago by knsam

Replying to tscrim: Thank you for the specific pointers.

For example, the INPUT: and OUTPUT: blocks should be:

INPUT:

- ``arg_name`` -- Description of argument

- ``second_arg`` -- Another description

OUTPUT:

Describing the output

Also, for multi-line doctests, it is just ..., not ....: (don't ask me why), I prefer EXAMPLES: (with the s no matter if it is 1 or many), and capitalized .. SEEALSO:: and such blocks.

  1. OK, I think I understand what you mean about INPUT. Irrespective of the number of inputs, you have a bulleted list. Is this what we want? (Otherwise, I am not sure what is wrong with them.)
  1. Secondly, on the devel mailing list, I have seen that Keshav Kini was saying that it is better to have ....: instead of ... (there is a also a ticket on trac: #10458). So, I am now lost.
  1. About, EXAMPLES, I'd go ahead and change. Capitalisation shall also be done.

A good check is to run sage -docbuild reference html (after rebuilding sage via sage -b) and look at the output in .../sage-branch/doc/output/html/... in your favorite web-browser (Firefox had some issues last I knew, this might have been fixed...).

Sure thing. Being a newbie that I am, my eyes are not trained enough to catch oddities. ;-)

Thank you for all the comments. I'll submit a patch to be applied on top of this. Thank you.

comment:11 in reply to:  10 Changed 10 years ago by tscrim

Replying to knsam:

  1. OK, I think I understand what you mean about INPUT. Irrespective of the number of inputs, you have a bulleted list. Is this what we want? (Otherwise, I am not sure what is wrong with them.)

That is correct.

  1. Secondly, on the devel mailing list, I have seen that Keshav Kini was saying that it is better to have ....: instead of ... (there is a also a ticket on trac: #10458). So, I am now lost.

You'll notice that the ticket is not yet into sage (its status would be "closed" if it was), so those doctests will currently fail (you can run sage -t filename1 filename2 ... from the terminal).

Thank you for all the comments. I'll submit a patch to be applied on top of this. Thank you.

Not a problem. You can just use the same patch (just make sure to check the "replace file if exists" during the attachments upload).

Best,
Travis

Changed 10 years ago by knsam

First Part.

Changed 10 years ago by knsam

Second and the last part

comment:12 Changed 10 years ago by knsam

Description: modified (diff)
Status: needs_workneeds_review

Changed 10 years ago by knsam

Not quite! Perhaps, the last!

comment:13 Changed 10 years ago by knsam

Description: modified (diff)

Hah! I have found one last glitch just now, which I do not how to fix:

x.__init__(...) initializes x; see help(type(x)) for signature

and

x.next() -> the next value, or raise StopIteration

which appear in the ExpressionIterator class, towards the end of the built html.

I don't know how to fix this.

comment:14 Changed 10 years ago by kcrisman

What are you trying to fix? By the way, the first patch is quite a bomb - I like the intent, but it could cause a lot of rebasing to be needed.

  • Also, :meth:bool isn't right, because bool isn't a method, it's a function.
  • And in taylor you dedented stuff that should have remained indented; there are two notations supported, and they should be under that listing.

That said, these are mostly going to be useful changes! Did you look at the built documentation to check whether they "look right"? Travis has the right idea there.

comment:15 Changed 10 years ago by tscrim

I believe both are coming from the fact that ExpressionIterator does not have any class-level docstring. Trying giving it something like

"""
Iterator class for expressions.
"""

and see if that fixes the problems. Also once you integrate Karl-Dieter's changes, I'll give it another review (also, more of a side-note, I would prefer to have one patch since the other two patches are very small). Thanks.

comment:16 Changed 10 years ago by kcrisman

Status: needs_reviewneeds_work

comment:17 Changed 10 years ago by kcrisman

Authors: Kenneth SmithKenneth Smith, Kannappan Sampath
Reviewers: Travis ScrimshawTravis Scrimshaw, Kannappan Sampath

comment:18 Changed 9 years ago by jdemeyer

Milestone: sage-5.11sage-5.12

comment:19 Changed 9 years ago by vbraun_spam

Milestone: sage-6.1sage-6.2

comment:20 Changed 9 years ago by vbraun_spam

Milestone: sage-6.2sage-6.3

comment:21 Changed 8 years ago by vbraun_spam

Milestone: sage-6.3sage-6.4

comment:22 Changed 5 weeks ago by mkoeppe

Milestone: sage-6.4
Note: See TracTickets for help on using tickets.