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

feat(interlinks): filter on py domain by default #254

Merged
merged 5 commits into from
Aug 29, 2023

Conversation

jmbuhr
Copy link
Contributor

@jmbuhr jmbuhr commented Aug 28, 2023

Types such as dict and list in the python standard library appear multiple times in the inventory. For example, for dict, there is the standard library https://docs.python.org/3.10/library/stdtypes.html#dict (the one we want), but also more obscure matches like https://docs.python.org/3.10/library/2to3.html?highlight=dict#to3fixer-dict.

Luckily, the stdtypes matches come up first. There is no guaranty for this, but I think it is fine to still link the first match and log a warning instead of not linking them.

While I was at it, I also made the module local functions local, renamed the inventory variable in the for loop to not shadow the global inventory and replaced deprecated quarto.utils functions with the appropriate quarto.log functions (had to get rid of those squiggly lines).

@machow
Copy link
Owner

machow commented Aug 28, 2023

Thanks for this--the multiple matches has been bugging a few different people, so this fix is super helpful! I'm looking back at some notes, and it seems like there might be two moves I missed originally with interlinks..

Ignoring std:doc entries

It looks like sphinx interlinks ignore std:doc entries by default, and duplicate entries often come from that inventory.

I wonder if we add an intersphinx_disabled_reftypes options, and the set the default to std:*, if that would get rid of most duplicate matches? (since python built-in API links are under the :py:* domain).

Does excluding entries with domain std in the interlinks filter work for you? Here's the relevant line of code. Glancing at the output of some site builds, it seems like most duplicates are from std.

More extreme option: filtering to keep only py domain entries

Thinking more, I wonder if a big piece I missed--that might remove most multiple matches--is specifying the interlinks to filter by the py: domain? (related to #221). I wonder if sphinx generate links from annotations in such a way that it looks for matches in the py domain (when python objects are being documented).

@jmbuhr
Copy link
Contributor Author

jmbuhr commented Aug 28, 2023

Indeed, for e.g. dict we find the following inventory items:

{
  dispname: "-"
  domain: "py"
  name: "dict"
  priority: "1"
  role: "class"
  uri: "https://docs.python.org/3.10/library/stdtypes.html#$"
}
{
  dispname: "Dictionary displays"
  domain: "std"
  name: "dict"
  priority: "-1"
  role: "label"
  uri: "https://docs.python.org/3.10/reference/expressions.html#$"
}
{
  dispname: "-"
  domain: "std"
  name: "dict"
  priority: "1"
  role: "2to3fixer"
  uri: "https://docs.python.org/3.10/library/2to3.html#to3fixer-$"
}

Filtering for the py domain as in the commit I just pushed properly narrows it down for these cases.

Copy link
Owner

@machow machow left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks great, thanks! I left a very small comment, that I think would let us keep using non-python links. If that tweak works, we should be ready to merge!

_extensions/interlinks/interlinks.lua Outdated Show resolved Hide resolved
@machow machow changed the title fix(interlinks): use first match if multiple matches are found feat(interlinks): filter on py domain by default Aug 29, 2023
@machow machow merged commit e02bcee into machow:main Aug 29, 2023
@machow
Copy link
Owner

machow commented Aug 29, 2023

Thanks again for submitting! In case you're interested in trying it out, I have a PR open to speed up the interlinks filter, by having it just read raw inventory files :) #253.

@has2k1 has2k1 mentioned this pull request Aug 30, 2023
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