Documentation for auto-generated index of functions

[ncohen]

This could have a place in the developer's manual, or it will be yet another dark feature.

Nathann

[ncohen]

New commits:

​757a9f4

trac #19482: Documentation for auto-generated index of functions

[jmantysalo]

I tried to follow instructions with categories/finite_posets.py, but got only

docstring of sage.categories.finite_posets:11: WARNING: The "csv-table" directive requires content; none supplied.

I guess that I should change __name__ to something. To what? And should it be told in these instructions?

[ncohen]

Yo,

I tried to follow instructions with categories/finite_posets.py, but got only

docstring of sage.categories.finite_posets:11: WARNING: The "csv-table" directive requires content; none supplied.

Would I be correct to guess that you followed the instructions for an index of *functions* in a file that does not define any?

In that file, I'd say that what you want are the methods of a specific class.

I guess that I should change __name__ to something.

Nono, this should work.

And should it be told in these instructions?

Let us first figure out what went wrong in your test.

Nathann

[jmantysalo]

Replying to ncohen:

Would I be correct to guess that you followed the instructions for an index of *functions* in a file that does not define any?

There is is_lattice(), is_poset_isomorphism() etc. defined in categories/finite_posets.py.

[ncohen]

I just checked my terminology, but it seems correct: ​https://en.wikipedia.org/wiki/Method_(computer_programming)

A 'method' is a function defined in a class. The functions are the functions defined in a module.

sage: import inspect
sage: P = Poset()
sage: inspect.ismethod(P.is_lattice)
True
sage: inspect.isfunction(P.is_lattice)
False

Nathann

[jmantysalo]

Replying to ncohen:

A 'method' is a function defined in a class. The functions are the functions defined in a module.

OK. Now I tried to read incidence_structures.py and copypaste it to finite_posets.py. It start with

r"""
Finite posets

List of functions:

{METHODS_OF_FinitePosets}
"""

and ends

from sage.misc.rest_index_of_methods import gen_rest_table_index
__doc__ = __doc__.format(METHODS_OF_FinitePosets=gen_rest_table_index(FinitePosets))

There might be no problem at all for clever people, but I do not understand.

[ncohen]

That's because of the syntax of python's format thing:

sage: "a is {a} and b is {b}".format(a=4,b=5)
'a is 4 and b is 5'

Then you have to know that __doc__ is the name of the module's docstring (the string that starts with 'Finite posets'. And then gen_rest_table_index does its job from the class that you give it.

I don't mind your telling me that it is not clear or has to be explained in the developer's manual: it would be cool if we don't have to explain how python works there, though.

But then perhaps those details should not appear in the developer's manual but rather on the module's doc of this rest_index_of_methods module.

Nathann

[vbraun]

I guess the question is whether we really want widespread use of a hack that does large-ish string processing at each import (including during Sage startup).

An example for how to do things right is ​http://docs.astropy.org/en/stable/nddata/index.html, the auto-generated class table at the bottom is created only once while running sphinx to build the docs.

[ncohen]

I guess the question is whether we really want widespread use of a hack that does large-ish string processing at each import (including during Sage startup).

Not at all. The question here is whether you want the interface of this code to be documented, so that people can use it.

How it is implemented is a different problem, and you are most welcome to help if you think that it should be written differently.

Nathann

[ncohen]

What you say is true, however. The way they do it (through Sphinx) is much cleaner. I haven't found the list of method of a given classes so far, but it's probably doable with their code.

[jmantysalo]

Back to this one...

I would not say "Optionally, the module's documentation can include a list - -", but something like "Module documentation should include a list of all functions. If there are only few of them, the list can be generated - -".

Maybe after example it could also say something like "The list will not contain functions that are deprecated or whose name start with underscore." I am not sure if this is good addition or just noise.

After "An index of all methods of a specific class - -" add "There is no automatic way to get a list of all functions defined in a module containing several classes."

"When a class/module defines many methods/functions it can become interesting - -" should be "When a class/module defines many methods/functions the list should be grouped thematically - -".

Otherwise I think that I have now understood how to use this and what can be done with automatic indexing.

[ncohen]

Yo !

I would not say "Optionally, the module's documentation can include a list - -", but something like "Module documentation should include a list of all functions. If there are only few of them, the list can be generated - -".

HMmmm.. Sorry for nitpicking, but I don't allow myself to write rules in our developer's manual without a poll on sage-devel. Around 0% of our modules follow this rule at the moment, and phrasing it like that might say that we could refuse branches because they don't have it.

Maybe after example it could also say something like "The list will not contain functions that are deprecated or whose name start with underscore." I am not sure if this is good addition or just noise.

Well, to me this is more documentation specific to the reference manual. There is a link toward the rest_index_of_methods module that people can follow, and see what is there. So what you say is necessary <somewhere> but I don't personall believe it must be here.

After "An index of all methods of a specific class - -" add "There is no automatic way to get a list of all functions defined in a module containing several classes."

Errr... This sentence is 'wrong', technically. A function, by definition, does not belong to a class. A method, by definition, belongs to a class. You want to say that you cannot have one big table that mixes together the methods of different classes? That's rather expected, isn't it?.. :-P

"When a class/module defines many methods/functions it can become interesting - -" should be "When a class/module defines many methods/functions the list should be grouped thematically - -".

Sorry, same reason. I don't want to use 'should' here. I don't mind making tools available, though I don't want to force people to use them. Especially when some don't like them.

Nathann

[vbraun]

Warning: this slows down the module import and should at most be used for modules that are lazily imported.

[ncohen]

See? "Can be used". Not "should be used" :-P

[jmantysalo]

In any case, this is better with this addition than without, so I mark this as positive review. But some comments:

[ By comment of vbraun in time of writing this I cancelled positive review. ]

Replying to ncohen:

After "An index of all methods of a specific class - -" add "There is no automatic way to get a list of all functions defined in a module containing several classes."

Errr... This sentence is 'wrong', technically. A function, by definition, does not belong to a class. A method, by definition, belongs to a class. You want to say that you cannot have one big table that mixes together the methods of different classes? That's rather expected, isn't it?.. :-P

Well, it is not that strange to think that functions, methods, classes, and whatevers, defined in one file can be shown in one index.

I would not say "Optionally, the module's documentation can include a list - -", but something like "Module documentation should include a list of all functions. If there are only few of them, the list can be generated - -".

HMmmm.. Sorry for nitpicking, but I don't allow myself to write rules in our developer's manual without a poll on sage-devel. Around 0% of our modules follow this rule at the moment, and phrasing it like that might say that we could refuse branches because they don't have it.

Actually I would like to have a kind of rule that new modules must have an index. Not necessarily made with this, but somehow.

But this has been discussed before without result. You want only rules that are really rules; I would like to have also recommendations of the form "Do X or say why you did not do."

[ncohen]

Well, it is not that strange to think that functions, methods, classes, and whatevers, defined in one file can be shown in one index.

What do you do when several classes have methods with the same name?

Actually I would like to have a kind of rule that new modules must have an index. Not necessarily made with this, but somehow.

But this has been discussed before without result. You want only rules that are really rules; I would like to have also recommendations of the form "Do X or say why you did not do."

Hmmmm... Well, then add a commit if you like?

Nathann

[jmantysalo]

Replying to ncohen:

Well, it is not that strange to think that functions, methods, classes, and whatevers, defined in one file can be shown in one index.

What do you do when several classes have methods with the same name?

Have a link with class name added. ... ok, ok, bad idea.

Hmmmm... Well, then add a commit if you like?

Maybe not for this one.

But anyways, should this be accepted or not? Volker, is a warning needed in this place, in the documentation of gen_rest...(), or in both? Or not at all?

[jmantysalo]

Replying to jmantysalo:

But anyways, should this be accepted or not? Volker, is a warning needed in this place, in the documentation of gen_rest...(), or in both? Or not at all?

Just pinging... What should we do with this?

[ncohen]

(test request from Jori. Please ignore)

[jmantysalo]

Getting back to this one... Volker, I still think that Sage with this patch is better than Sage without. As Nathann said, without this we have another dark feature.

[vbraun]

Startup time for Sage is also an issue, we are basically already borderline too slow to start. Doing more stuff at startup is not going to help. Related, the proposed syntax of slapping some magic stuff at the bottom of each page isn't something that you should do if you had a proper implementation. If you do it right (did you look at the astropy example?) then you wouldn't need the magic __doc__ = ... anyways. IMHO we should have a good implementation first before encouraging others to use it.

[jmantysalo]

Replying to vbraun:

IMHO we should have a good implementation first before encouraging others to use it.

OK. And I think that we all agree that Sphinx gives headache to everyone.

There is now five files that uses this (2 in combinat, 2 in coding, 1 in graphs). Should we add some comment to those, just before {INDEX_OF_FUNCTIONS} to tell developers what is going on?

[jmantysalo]

As there does not seem to be common view, and Nathann has faded from developers (sad), I mark this as wontfix/positive. Volker, please revert if you have other opinion.

[embray]

Determined to be invalid/duplicate/wontfix (closing as "wontfix" as a catch-all resolution).