Add option `matlab_auto_link`

rdzman commented 1 year ago

This is preliminary work to address #178.

As stated there, currently:

Added a matlab_auto_link config option that currently takes 2 options, "see_also" and "all".
Added a ref_role() method to MatObject types to return the role to use for references to each type of object.
(now fixed) Need to update the regex to match only whole words. Right now if you have myClass and myClassFoo, it will replace myClassFoo with :class:`myClass`Foo, which isn't quite what we're looking for.

joeced commented 1 year ago

It would be great if you could extend a class in for instance test_autodoc with a See Also section.

Also consider other valid "See Also" uses.

Functions on the next line.

...
Other documentation...

See Also
   OtherFunction

Napoleon style docstrings: https://www.sphinx-doc.org/en/master/usage/extensions/napoleon.html, i.e See Also:

...
Other documentation...

See Also:
   OtherFunction

joeced commented 1 year ago

Do you think it's ready for merging? It look okay to me.

rdzman commented 1 year ago

I don't think we're quite ready, even with the basic "see also" functionality. And there are definitely additional considerations, especially with auto-linking more than "see also" elements.

But first, it currently has issues if matlab_short_links if False. For example, if I add matlab_auto_link = "see_also" to tests/roots/test_autodoc.conf.py and run make html, I get warnings like mat:class reference target not found: ClassExample.

And, sure enough, if you open tests/roots/test_autodoc/_build/html/index_root.html the corresponding "see also" items are styled as code, but they are not actually links. If I turn on matlab_short_links then it generates a valid link. But with matlab_short_links off, even if I change the see also text to target.ClassExample it will not work, because the auto-linking code is still searching for ClassExample (the corresponding key in the entities_table), not target.ClassExample.

Any suggestions for how to fix this? Should I be searching for something other than the key?

rdzman commented 1 year ago

Other considerations, in no particular order ...

Currently any entry in a "see also" line that is not found in entities_table, e.g. Matlab built-in functions/classes, are left untouched, which means they are not even styled as code in the output. What do you think about wrapping them in double-backquotes? Or is there another Sphinx role that would be more appropriate?
As mentioned in item 1 (b) under Background in #178, it would be nice to link properties and methods listed in the class docstring under the corresponding headings, as MATLAB's doc and help commands do.
- First, what is the best way to get a list of the properties and methods of a particular MatClass object
- Is it possible to have, for example, a link to MyClass.mymethod the displays as only mymethod()? Would :meth: have to be made smarter to accomplish that?
- And if that's not possible, what do you think of at least auto-wrapping them in double-backquotes or something to make them display as code? If I recall, doing that in the source doc breaks MATLAB's auto linking.

rdzman commented 1 year ago

But first, it currently has issues if matlab_short_links if False.

Ok, I think I fixed this problem. It was an issue with my regex, not what I thought. It was autolinking twice (e.g. once for target.ClassExample then again for ClassExample, turning ...

target.ClassExample

... into ...

:class:`target.:class:`ClassExample``

rdzman commented 1 year ago

I'm working on refactoring the "see also" autolinking to search the entities_table for each entry in the "see also" line, as opposed to the other way around. This allows the option to do something with entries that are not found, like wrap them in double backquotes.

However, I'm not sure I understand the entries in the entities_table. For example, for target/+package/ClassBar, it appears there are entries for:

target.+package.ClassBar
package.ClassBar

But I think the link that works has to be :class:`target.package.ClassBar`. That is, with target, but without +. Should I just strip the + from the keys when I search for a match?

joeced commented 1 year ago

I'm working on refactoring the "see also" autolinking to search the entities_table for each entry in the "see also" line, as opposed to the other way around. This allows the option to do something with entries that are not found, like wrap them in double backquotes.

However, I'm not sure I understand the entries in the entities_table. For example, for target/+package/ClassBar, it appears there are entries for:
target.+package.ClassBar
package.ClassBar

So in entities_table we have all valid names to an entity (module (folder), class or function). I first parse the regular was, and then I add a short name to the same dictionary, the short name points to the same object.

But I think the link that works has to be :class:`target.package.ClassBar`. That is, with target, but without +. Should I just strip the + from the keys when I search for a match?

It depends, in https://github.com/sphinx-contrib/matlabdomain/blob/81f5f17f6306b2b56a743cb67e9f12a97c710596/sphinxcontrib/mat_documenters.py#LL769C20-L769C20 I had to do another form for the base class links to make to work both with short and long names. A Sphinx reference can also be written as: `:class:``title ``` as documented here: https://www.sphinx-doc.org/en/master/usage/restructuredtext/domains.html#cross-referencing-syntax.

Maybe this can help you?

rdzman commented 1 year ago

Ah yes, that link about Sphinx cross-referencing is very helpful. I was not aware of that flexibility. And seeing how you handled base class links is helpful too. I'm thinking I might even break some of that code out into its own method so I can reuse it.

But the first step for me is to be able to reliably look up an entity given a name, presumably a name in the form that would be used to display it. I think that means:

package.Class for matlab_short_links = True (what I intend to use)
target.+package.Class for matlab_keep_package_prefix = True
target.package.Class for matlab_keep_package_prefix = False

I think the first two are easy, since the name should already be a key in the entities_table dictionary. But the last one seems trickier. Am I thinking about this correctly?

Any suggestions?

rdzman commented 1 year ago

The approach I took is to build a dictionary mapping the names without + on packages to names with +. Currently it does it on the fly each time it hits a docstring with a "see also" line. I'm sure there's a better way.

But it appears to work.

joeced commented 1 year ago

Great job so far. I saw that you had the long names in the See Also section in the m-files. Can you make it work with MATLAB names?

rdzman commented 1 year ago

I believe it does work both ways, depending on the setting of the matlab_short_links option.

Would you like me to turn matlab_short_links on in tests/roots/test_autodoc/conf.py and update the expected outputs in the tests so they pass? Or should we really be duplicating that whole set of tests with matlab_short_links on?

rdzman commented 1 year ago

To clarify, currently both the long names and the short names get wrapped with :class:`<name>` (since they are both present in the entities_table), but only one of the two will actually result in a functioning link, depending on the value of matlab_short_links.

rdzman commented 1 year ago

As mentioned in item 1 (b) under Background in auto-link functions, classes and members in docstring text #178, it would be nice to link properties and methods listed in the class docstring under the corresponding headings, as MATLAB's doc and help commands do.

I think I know how to do this now, so I'm planning to take a crack at it.

This would give us 3 independent types of auto-linking:

In "see also" lines, auto-link known names (those found in entities_table) and double-backquote anything else. (done)
In a class docstring, auto-link the first name in each line under "MyClass Properties:" or "MyClass Methods:".
Auto-link each name in entities_table anywhere it is found in any docstring. (done)

How do you think we should structure the options for the user?

We could use three individual options, but I can't imagine using the 3rd one and not the first two. Or we could have a single option with multiple values like we do now, but something like "normal" (for see also and member) and "all" for all three.

Do you have a preference?

joeced commented 1 year ago

As mentioned in item 1 (b) under Background in auto-link functions, classes and members in docstring text #178, it would be nice to link properties and methods listed in the class docstring under the corresponding headings, as MATLAB's doc and help commands do.

I think I know how to do this now, so I'm planning to take a crack at it.

This would give us 3 independent types of auto-linking:
* In "see also" lines, auto-link known names (those found in `entities_table`) and double-backquote anything else. _(done)_

* In a class docstring, auto-link the first name in each line under "MyClass Properties:" or "MyClass Methods:".

* Auto-link each name in `entities_table` anywhere it is found in any docstring. _(done)_
How do you think we should structure the options for the user?

We could use three individual options, but I can't imagine using the 3rd one and not the first two. Or we could have a single option with multiple values like we do now, but something like "normal" (for see also and member) and "all" for all three.

Do you have a preference?

I think a single option to encompass them all would be preferable. We already have lots of options, and documenting them all takes time and effort. Plus, there may be combination that we don't test. I think your suggestion to use: None, Normal/Regular, All seems reasonable.

Looking forward when you get further. Keep up the good work :)

rdzman commented 1 year ago

I think this is pretty much ready for a thorough review. It now has 3 possible values for matlab_auto_link:

None (default) - no auto-linking
"basic" - auto-link classes and functions in "See also" lines and property and method names in "MyClass Properties:" and "MyClass Methods:" sections of a class docstring.
"all" - everything in "basic" plus linking of known function and class names anywhere else they are found in any docstrings

I also have an identical branch that has been re-based on the current master. Would you like me to force-push that here?

rdzman commented 1 year ago

Ok, I've added a 4th type of auto-linking, included when matlab_auto_link = "all". In any class, property or method docstring, if a name (followed by ()) is found that matches a method of the corresponding class, it is also auto-linked.

I could do something similar with property names, but I think it would need some sort of required markup (like the () for methods), such as enclosing the name in quotes or something. Otherwise I wouldn't want to see the result if someone had a property named a, for example.

rdzman commented 1 year ago

On another note, since I was getting some broken links (both base class links and auto-links) with matlab_short_links = True, I added a version of the autodoc tests to test everything with that option turned on.

I also fixed the bug that was causing the broken links.

We are currently using class links of the form:

:class:`name <target>`

But shouldn't the name and the target always be the same for base class links and auto-links? I think so. So I'm planning to change them to just ...

:class `target`

Then I think I'll really be done with this PR. Once again, is it ok with you for me to force-push a version of this branch that's been rebased on the current master? (I've always preferred rebasing before doing a merge so that all merges are fast-forward.)

joeced commented 1 year ago

This is excellent work. I'm really impressed :). And yes please force push the branch you want me to merge into master.

I'll have time to review it in detail on Tuesday.

rdzman commented 1 year ago

👍 Thanks.

sphinx-contrib / matlabdomain

Add option `matlab_auto_link` #183