Opened 5 years ago
Closed 4 years ago
#19482 closed enhancement (wontfix)
Documentation for auto-generated index of functions
Reported by: | ncohen | Owned by: | |
---|---|---|---|
Priority: | major | Milestone: | sage-duplicate/invalid/wontfix |
Component: | documentation | Keywords: | |
Cc: | Merged in: | ||
Authors: | Nathann Cohen | Reviewers: | Jori Mäntysalo |
Report Upstream: | N/A | Work issues: | |
Branch: | u/ncohen/19482 (Commits) | Commit: | 757a9f4a908db4be170f28a96c000a1358ac9f2d |
Dependencies: | Stopgaps: |
Description
This could have a place in the developer's manual, or it will be yet another dark feature.
Nathann
Change History (26)
comment:1 Changed 5 years ago by
- Branch set to u/ncohen/19482
- Commit set to 757a9f4a908db4be170f28a96c000a1358ac9f2d
- Status changed from new to needs_review
comment:2 follow-up: ↓ 3 Changed 5 years ago by
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?
comment:3 in reply to: ↑ 2 ; follow-up: ↓ 4 Changed 5 years ago by
Yo,
I tried to follow instructions with
categories/finite_posets.py
, but got onlydocstring 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
comment:4 in reply to: ↑ 3 Changed 5 years ago by
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
.
comment:5 follow-up: ↓ 6 Changed 5 years ago by
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
comment:6 in reply to: ↑ 5 Changed 5 years ago by
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.
comment:7 Changed 5 years ago by
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
comment:8 follow-up: ↓ 9 Changed 5 years ago by
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.
comment:9 in reply to: ↑ 8 Changed 5 years ago by
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
comment:10 Changed 5 years ago by
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.
comment:11 follow-up: ↓ 14 Changed 5 years ago by
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.
comment:12 Changed 5 years ago by
- Reviewers set to Jori Mäntysalo
comment:13 Changed 5 years ago by
- Milestone changed from sage-6.10 to sage-7.0
comment:14 in reply to: ↑ 11 ; follow-up: ↓ 17 Changed 5 years ago by
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
comment:15 Changed 5 years ago by
Warning: this slows down the module import and should at most be used for modules that are lazily imported.
comment:16 Changed 5 years ago by
See? "Can be used". Not "should be used" :-P
comment:17 in reply to: ↑ 14 ; follow-up: ↓ 18 Changed 5 years ago by
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."
comment:18 in reply to: ↑ 17 ; follow-up: ↓ 19 Changed 5 years ago by
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
comment:19 in reply to: ↑ 18 ; follow-up: ↓ 20 Changed 5 years ago by
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?
comment:20 in reply to: ↑ 19 Changed 5 years ago by
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?
comment:21 Changed 5 years ago by
(test request from Jori. Please ignore)
comment:22 Changed 5 years ago by
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.
comment:23 follow-up: ↓ 24 Changed 5 years ago by
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.
comment:24 in reply to: ↑ 23 Changed 5 years ago by
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?
comment:25 Changed 5 years ago by
- Milestone changed from sage-7.0 to sage-duplicate/invalid/wontfix
- Status changed from needs_review to positive_review
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.
comment:26 Changed 4 years ago by
- Resolution set to wontfix
- Status changed from positive_review to closed
Determined to be invalid/duplicate/wontfix (closing as "wontfix" as a catch-all resolution).
New commits:
trac #19482: Documentation for auto-generated index of functions