Opened 3 years ago

Closed 3 years ago

#19067 closed enhancement (fixed)

Incomplete sentences in the automatically generated documentation

Reported by: ncohen Owned by:
Priority: major Milestone: sage-6.9
Component: documentation Keywords:
Cc: jmantysalo, jsrn Merged in:
Authors: Nathann Cohen Reviewers: Marc Mezzarobba
Report Upstream: N/A Work issues:
Branch: 3728de7 (Commits) Commit: 3728de720c632a5dabc09824000e6818c5716274
Dependencies: Stopgaps:

Description (last modified by ncohen)

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

Change History (35)

comment:1 Changed 3 years ago by ncohen

  • Branch set to u/ncohen/19067
  • Description modified (diff)
  • Status changed from new to needs_review

comment:2 Changed 3 years ago by git

  • Commit set to 329554600f4765cd6071f1255625ea94243da639

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

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

comment:3 follow-up: Changed 3 years ago by jmantysalo

[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.

comment:4 Changed 3 years ago by git

  • Commit changed from 329554600f4765cd6071f1255625ea94243da639 to 4bc03ab80228f4f09f4de094390d275e614ab3d6

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

4bc03abtrac #19067: Change delimiter

comment:5 in reply to: ↑ 3 Changed 3 years ago by ncohen

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

comment:6 Changed 3 years ago by jmantysalo

  • Status changed from needs_review to needs_work

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.

comment:7 Changed 3 years ago by jmantysalo

  • Status changed from needs_work to needs_review

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

comment:8 Changed 3 years ago by git

  • Commit changed from 4bc03ab80228f4f09f4de094390d275e614ab3d6 to f1b92bc9dbfb850aefedee265047a47b8728ded6

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

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

comment:9 follow-up: Changed 3 years ago by mmezzarobba

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!

comment:10 in reply to: ↑ 9 ; follow-up: Changed 3 years ago by ncohen

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

comment:11 in reply to: ↑ 10 ; follow-up: Changed 3 years ago by jmantysalo

Replying to ncohen:

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?

comment:12 in reply to: ↑ 11 Changed 3 years ago by ncohen

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.

comment:13 Changed 3 years ago by mmezzarobba

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'
...

comment:14 follow-up: Changed 3 years ago by mmezzarobba

...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?

comment:15 Changed 3 years ago by git

  • Commit changed from f1b92bc9dbfb850aefedee265047a47b8728ded6 to 317d6d61dd030e5df134896b5556408fda14d8df

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

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

comment:16 in reply to: ↑ 14 Changed 3 years ago by ncohen

...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

comment:17 follow-up: Changed 3 years ago by jmantysalo

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! ;=))

comment:18 in reply to: ↑ 17 ; follow-up: Changed 3 years ago by ncohen

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

comment:19 in reply to: ↑ 18 ; follow-up: Changed 3 years ago by jmantysalo

Replying to ncohen:

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.

comment:20 in reply to: ↑ 19 Changed 3 years ago by ncohen

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

comment:21 follow-up: Changed 3 years ago by jmantysalo

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."

comment:22 in reply to: ↑ 21 ; follow-up: Changed 3 years ago by ncohen

"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

comment:23 in reply to: ↑ 22 ; follow-up: Changed 3 years ago by jmantysalo

Replying to ncohen:

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.

comment:24 in reply to: ↑ 23 Changed 3 years ago by ncohen

<shakes hands>

No prob. Sphinx is a mess anyway.

comment:25 Changed 3 years ago by mmezzarobba

  • Reviewers set to Marc Mezzarobba
  • Status changed from needs_review to positive_review

comment:26 Changed 3 years ago by ncohen

Thank you!

Nathann

comment:27 Changed 3 years ago by vbraun

  • Status changed from positive_review to needs_work

PDF documentation doesn't build

comment:28 Changed 3 years ago by git

  • Commit changed from 317d6d61dd030e5df134896b5556408fda14d8df to 3728de720c632a5dabc09824000e6818c5716274

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

3728de7trac #19067: Typo.

comment:29 Changed 3 years ago by ncohen

  • Status changed from needs_work to positive_review

comment:30 follow-up: Changed 3 years ago by ncohen

  • Cc jsrn added

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

comment:31 in reply to: ↑ 30 ; follow-up: Changed 3 years ago by jmantysalo

Replying to ncohen:

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"?

comment:32 in reply to: ↑ 31 ; follow-up: Changed 3 years ago by ncohen

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

comment:33 Changed 3 years ago by jsrn

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...

comment:34 in reply to: ↑ 32 Changed 3 years ago by jmantysalo

Replying to ncohen:

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().

Last edited 3 years ago by jmantysalo (previous) (diff)

comment:35 Changed 3 years ago by vbraun

  • Branch changed from u/ncohen/19067 to 3728de720c632a5dabc09824000e6818c5716274
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.