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

Incomplete sentences in the automatically generated documentation #19067

Closed
nathanncohen mannequin opened this issue Aug 21, 2015 · 40 comments
Closed

Incomplete sentences in the automatically generated documentation #19067

nathanncohen mannequin opened this issue Aug 21, 2015 · 40 comments

Comments

@nathanncohen
Copy link
Mannequin

nathanncohen mannequin commented Aug 21, 2015

This branch fixes the following bug reported by Jori on #19061

sage: from sage.misc.rest_index_of_methods import gen_rest_table_index
sage: def a():
....:     r'''
....:     Here is a very very very long sentence
....:     that spans on several lines.
....:
....:     EXAMP...
....:     '''
....:     print "hey"
sage: 'Here is a very very very long sentence that spans on several lines' in gen_rest_table_index([a])
False
sage: 'Here is a very very very long sentence' in gen_rest_table_index([a])
True

Nathann

CC: @jm58660 @johanrosenkilde

Component: documentation

Author: Nathann Cohen

Branch/Commit: 3728de7

Reviewer: Marc Mezzarobba

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

@nathanncohen nathanncohen mannequin added this to the sage-6.9 milestone Aug 21, 2015
@nathanncohen
Copy link
Mannequin Author

nathanncohen mannequin commented Aug 21, 2015

Branch: u/ncohen/19067

@nathanncohen

This comment has been minimized.

@nathanncohen nathanncohen mannequin added the s: needs review label Aug 21, 2015
@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Aug 21, 2015

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

3295546trac #19067: Incomplete sentences in the automatically generated documentation

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Aug 21, 2015

Commit: 3295546

@jm58660
Copy link
Mannequin

jm58660 mannequin commented Aug 21, 2015

comment:3
[combinat ] reading sources... [100%] sage/combinat/yang_baxter_graph
[combinat ] loading cross citations...
[combinat ] /home/jm58660/sage/local/lib/python2.7/site-packages/sage/combinat/designs/difference_family.py:docstring of sage.combinat.designs.difference_family:8: ERROR: "csv-table" widths do not match the number of columns in table (6).
[combinat ] .. csv-table::
[combinat ] :class: contentstable
[combinat ] :widths: 30, 70
[combinat ] :delim: |
[combinat ] :func:`~sage.combinat.designs.difference_family.block_stabilizer` | Compute the left stabilizer of the block ``B`` under the action of ``G``.

A bug? I don't know.

Should this be marked as dependency to #19061? It is quite meaningless without this, even if technically not dependig.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Aug 21, 2015

Changed commit from 3295546 to 4bc03ab

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Aug 21, 2015

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

4bc03abtrac #19067: Change delimiter

@nathanncohen
Copy link
Mannequin Author

nathanncohen mannequin commented Aug 21, 2015

comment:5

The problem was caused by a | character that appeared in the description of a function. The trouble is that | is the splitting character in those docstrings.

It is fixed.

Should this be marked as dependency to #19061?

With this last commit, I ready to bet that this ticket is now in conflict with #19061. I will have to fix the conflicts with a merge, but I would prefer this branch to be reviewed first, in order to avoid having to merge and re-merge things several times in case this review requires other changes.

Nathann

@jm58660
Copy link
Mannequin

jm58660 mannequin commented Sep 1, 2015

comment:6

This seems to fail if I change one-line description

Returns the value of the M\"obius function - -
Returns the value of the Möbius function - -

in hasse_diagram.py.

@jm58660 jm58660 mannequin added s: needs work and removed s: needs review labels Sep 1, 2015
@jm58660
Copy link
Mannequin

jm58660 mannequin commented Sep 1, 2015

comment:7

Duh. This is a bug, but not related to this ticket. Sorry.

@jm58660 jm58660 mannequin added s: needs review and removed s: needs work labels Sep 1, 2015
@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Sep 1, 2015

Changed commit from 4bc03ab to f1b92bc

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Sep 1, 2015

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

91a84f2trac #19067: Merged with 6.9.beta4
f1b92bctrac #19067: Trailing whitespace

@mezzarobba
Copy link
Member

comment:9

Do I understand correctly that the problem with | will come back if a docstring contains @? I don't like that much, especially now that @ is becoming a Python operator!

@nathanncohen
Copy link
Mannequin Author

nathanncohen mannequin commented Sep 3, 2015

comment:10

Do I understand correctly that the problem with | will come back if a docstring contains @?

Only if the first line of the docstring contains it.

I don't like that much, especially now that @ is becoming a Python operator!

Well, then we will have to find a way around, i.e. expose a parameter to change the delimiter manually, if needed. This being said, I am no magician: I looked at the table of ASCII characters and tried to figure out one that I could use. I picked that one because I thought it was okay, but it can be whatever else you prefer.

Nathann

@jm58660
Copy link
Mannequin

jm58660 mannequin commented Sep 4, 2015

comment:11

Replying to @nathanncohen:

I looked at the table of ASCII characters and tried to figure out one that I could use.

Just to confirm: the character is required to be in ASCII, not even ISO-8859-1?

Can it be non-printing character, i.e. some code between 1 and 31?

@nathanncohen
Copy link
Mannequin Author

nathanncohen mannequin commented Sep 4, 2015

comment:12

Just to confirm: the character is required to be in ASCII, not even ISO-8859-1?

Can it be non-printing character, i.e. some code between 1 and 31?

I do not know more than you do on this matter.

@mezzarobba
Copy link
Member

comment:13

I guess the natural choice would be U+001F (ASCII unit separator), but it doesn't seem to work:

...
[graphs   ] Exception occurred:
[graphs   ] File "/home/marc/co/sage/local/lib/python2.7/site-packages/docutils/parsers/rst/directives/__init__.py", line 303, in unicode_code
[graphs   ] if code.isdigit():                  # decimal number
[graphs   ] AttributeError: 'NoneType' object has no attribute 'isdigit'
...

@mezzarobba
Copy link
Member

comment:14

...So I guess the fix on this ticket is better than nothing. But what about also adding a warning in the documentation of rest_index_of_methods.py?

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Sep 7, 2015

Changed commit from f1b92bc to 317d6d6

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Sep 7, 2015

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

18baa7etrac #19067: Merged with 6.9.beta5
317d6d6trac #19067: Warning

@nathanncohen
Copy link
Mannequin Author

nathanncohen mannequin commented Sep 7, 2015

comment:16

...So I guess the fix on this ticket is better than nothing. But what about also adding a warning in the documentation of rest_index_of_methods.py?

Is it okay like that?

Nathann

@jm58660
Copy link
Mannequin

jm58660 mannequin commented Sep 7, 2015

comment:17

Could be better, if you would also warn against using non-accii characters in oneliners.

(Think if I happen to found a great theorem! And then Sage can not handle "Return Mäntysalo's foo of the bar"! Not even have a warning! ;=))

@nathanncohen
Copy link
Mannequin Author

nathanncohen mannequin commented Sep 7, 2015

comment:18

Could be better, if you would also warn against using non-accii characters in oneliners.

I do not think that this branch has any effect on the matter. If you experience that it does, please propose a commit.

Nathann

@jm58660
Copy link
Mannequin

jm58660 mannequin commented Sep 7, 2015

comment:19

Replying to @nathanncohen:

Could be better, if you would also warn against using non-accii characters in oneliners.

I do not think that this branch has any effect on the matter.

True. That is what I noticed in comment 7. But should there be a warning about non-ascii characters, now when there is about @?

Not a big deal, of course. So don't let this prevent giving positive review.

@nathanncohen
Copy link
Mannequin Author

nathanncohen mannequin commented Sep 7, 2015

comment:20

True. That is what I noticed in comment 7. But should there be a warning about non-ascii characters, now when there is about @?

Could you say clearly which kind of warning you expect to see? I cannot guess it.

Nathann

@jm58660
Copy link
Mannequin

jm58660 mannequin commented Sep 7, 2015

comment:21

Hmmm... something like

"First sentence in the documentation of a function may not contain the '@' character, nor any non-ascii character.

First restriction is because the ReST tables returned by this function use '@' as a delimiter for cells. Second restriction follows from Sphinx internals."

@nathanncohen
Copy link
Mannequin Author

nathanncohen mannequin commented Sep 7, 2015

comment:22

"First sentence in the documentation of a function may not contain the '@' character, nor any non-ascii character.

First restriction is because the ReST tables returned by this function use '@' as a delimiter for cells. Second restriction follows from Sphinx internals."

The management of non-ascii characters has nothing to do with this branch. If you write a non-ascii character in the documentation of a function, sphinx will complain even if this code is not used. Similarly, if you add # -*- coding: utf-8 -*- at the beginning of your files, then non-ascii characters will work well, regadless of whether you use this code or not.

Nathann

@jm58660
Copy link
Mannequin

jm58660 mannequin commented Sep 7, 2015

comment:23

Replying to @nathanncohen:

Similarly, if you add # -*- coding: utf-8 -*- at the beginning of your files, then non-ascii characters will work well, regadless of whether you use this code or not.

Now I understood. There was # -*- coding: utf-8 -*- at posets.py but not in hasse_diagram.py. And I thank that the reason for the difference was that latter one uses {INDEX_OF_FUNCTIONS}.

Sorry.

@nathanncohen
Copy link
Mannequin Author

nathanncohen mannequin commented Sep 7, 2015

comment:24

No prob. Sphinx is a mess anyway.

@mezzarobba
Copy link
Member

Reviewer: Marc Mezzarobba

@nathanncohen
Copy link
Mannequin Author

nathanncohen mannequin commented Sep 9, 2015

comment:26

Thank you!

Nathann

@vbraun
Copy link
Member

vbraun commented Sep 9, 2015

comment:27

PDF documentation doesn't build

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Sep 9, 2015

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

3728de7trac #19067: Typo.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Sep 9, 2015

Changed commit from 317d6d6 to 3728de7

@nathanncohen
Copy link
Mannequin Author

nathanncohen mannequin commented Sep 9, 2015

comment:30

That came from a \\ instead of \ in coding/guava.py.

The thing is that the docstrings of the code constructors do not respect the convention that the first line must be a short sentence describing the function, and as a result the first paragraph contains a LOT of text (including some latex).

Previously, only one (broken) sentence was displayed in the index. At least now the "sentence" is complete.

Nathann

@jm58660
Copy link
Mannequin

jm58660 mannequin commented Sep 10, 2015

comment:31

Replying to @nathanncohen:

The thing is that the docstrings of the code constructors do not respect the convention that the first line must be a short sentence describing the function - -

A meta-ticket about that? Do you have an easy way to get a list of those? I mean something like "oneliners with more than one dot" or "oneliners with more then 150 characters"?

@nathanncohen
Copy link
Mannequin Author

nathanncohen mannequin commented Sep 10, 2015

comment:32

A meta-ticket about that?

I do not want to see a meta-ticket again in my life.

Do you have an easy way to get a list of those?

No. I guess it has to be written "from scratch", or else it must be some long regexp.

I mean something like "oneliners with more than one dot" or "oneliners with more then 150 characters"?

Something line that.

Nathann

@johanrosenkilde
Copy link
Contributor

comment:33

Most of the doc in sage/coding "needs a bit of work" to put it mildly. But I guess that's for a different ticket, and that your fix to make things compile is all that should go into this one...

@jm58660
Copy link
Mannequin

jm58660 mannequin commented Sep 10, 2015

comment:34

Replying to @nathanncohen:

No. I guess it has to be written "from scratch", or else it must be some long regexp.

What? Long regexp? Those are always fun!

But actually this is easier with some bash magic. Here is one example:

(while egrep -m 1 '^    def [^_]'; do head -10 | egrep -m 1 -B 10 '^$' | tr '\n' ' '; echo; done) <
src/sage/combinat/posets/posets.py | tr -d '`' | egrep '.{250,}' -B 1 | egrep '^    def [^_]'

This founds level_sets() and coxeter_transformation().

@vbraun
Copy link
Member

vbraun commented Sep 10, 2015

Changed branch from u/ncohen/19067 to 3728de7

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

3 participants