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

Add option matlab_auto_link #183

Merged
merged 31 commits into from
May 30, 2023
Merged

Add option matlab_auto_link #183

merged 31 commits into from
May 30, 2023

Conversation

rdzman
Copy link
Contributor

@rdzman rdzman commented Apr 28, 2023

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
Copy link
Collaborator

joeced commented May 1, 2023

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
Copy link
Collaborator

joeced commented May 2, 2023

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

@rdzman
Copy link
Contributor Author

rdzman commented May 2, 2023

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
Copy link
Contributor Author

rdzman commented May 2, 2023

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 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.
    • 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
Copy link
Contributor Author

rdzman commented May 4, 2023

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
Copy link
Contributor Author

rdzman commented May 4, 2023

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
Copy link
Collaborator

joeced commented May 6, 2023

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
Copy link
Contributor Author

rdzman commented May 12, 2023

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
Copy link
Contributor Author

rdzman commented May 12, 2023

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
Copy link
Collaborator

joeced commented May 13, 2023

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
Copy link
Contributor Author

rdzman commented May 16, 2023

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 added 13 commits May 17, 2023 15:57
Currently just True or False, default is False.

In the future we may want to change this to a string with possible values like "see_also", "all" and maybe something inbetween,
Returns a string with the role to be used for references to this type of object (e.g. "class", "func", "meth", "attr").
Changed `matlab_auto_link` option to string value, currently with "see_also" or "all" as valid options.
…Methods:".

These are headings in class docstrings that are recognized by MATLAB's `help` and `doc`.
Add negative look-ahead for ` or . or +

Avoids turning ...
:class:`target.myClass`
... into...
:class:`target.:class:`myClass``
etc.
Look for each entity on “see also” line in the table, rather than the other way around.
Adds double-backquoting of unknown entities.
@rdzman
Copy link
Contributor Author

rdzman commented May 17, 2023

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
Copy link
Contributor Author

rdzman commented May 18, 2023

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
Copy link
Collaborator

joeced commented May 22, 2023

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
Copy link
Contributor Author

rdzman commented May 24, 2023

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?

Requires matlab_auto_link = "all" and the method name must appear with a trailing () to be auto-linked.
This affected both base class links and auto-linking. The issue was that package names were not being included in the link target.
@rdzman
Copy link
Contributor Author

rdzman commented May 26, 2023

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
Copy link
Contributor Author

rdzman commented May 26, 2023

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

This makes the visible names consistent with the `matlab_short_links` and `matlab_keep_package_prefix` options.
@joeced
Copy link
Collaborator

joeced commented May 27, 2023

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.

@joeced joeced merged commit f77fb9d into sphinx-contrib:master May 30, 2023
@rdzman
Copy link
Contributor Author

rdzman commented May 30, 2023

👍 Thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants