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

Intersphinx role (2) #9822

Merged
merged 18 commits into from
Jan 16, 2022
Merged

Intersphinx role (2) #9822

merged 18 commits into from
Jan 16, 2022

Conversation

jakobandersen
Copy link
Contributor

@jakobandersen jakobandersen commented Nov 6, 2021

Extension of #9062 (not sure if there is a good way simply to reuse that PR somehow)

Feature or Bugfix

  • Feature

Purpose

Continuation of the work by @tk0miya in #9062 to add a role for performing explicit intersphinx lookups.

Detail

  • the role(-prefix) is called external. I think it is ok to have a bit more verbose than just ext to make it stand out that this is not an ordinary cross-reference.

  • Add intersphinx role #9062 (comment), when a specific inventory lookup is requested I opted for + as the delimiter. When I tried the other options the inventory name sort of looked like part of the domain/reftype name instead of a separate item. Semantically a + also reads nicely as :external:python+py:class: indeed is requesting a lookup constrained to the python project and constrained to py:class objects.

  • the role will not fall back to the local project. It will warn if the lookup does not go well:

    • if the inventory name doesn't exist,
    • if the reftype/domain:reftype doesn't exist as a role (taking default-domain and the special std domain shorthands into acocunt), and
    • if a/the inventory doesn't contain the target.
  • it looks like Docutils mandates that role names are made lower-case before the role function is called, which is not good when an inventory name is present. So I haxed the original role name into the role processing. Not sure if there is a better way. Having the inventory name as part of the target text is problematic as intersphinx has no knowledge of the target format, so extracting information from it seems brittle.

  • Deprecating the old style prefixing will be done in a subsequent PR, but I think I got the intersphinx docs fully updated to mention this role instead.

Relates

sphinx/ext/intersphinx.py Outdated Show resolved Hide resolved

- ``:external:invname+domain:reftype:`target```,
e.g., ``:external:python+py:class:`zipfile.ZipFile```, or
Copy link
Member

Choose a reason for hiding this comment

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

When I saw this notation for the first time, I felt using + sign is cool. But this example makes my opinion opposite.

In my eyes, :external:python+doc: is good, but :external:python+py:class: is not so good. I guess the + sign confuses me. So I found 3 parts external, python+py and class from the latter example.

How about changing this notation?

  • :external+python:py:class:
    • Add a invname as a suffix of the "external" part.
    • It's almost similar to the current idea.
  • :external@python:py:class:
    • Add a invname as a suffix of the "external" part using a @ sign.
    • @ sign is usually used to represent a location because it's equivalent to "at".
    • It looks like a bit complicated.

Copy link
Contributor Author

@jakobandersen jakobandersen Jan 15, 2022

Choose a reason for hiding this comment

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

I agree, the + looks like it has "higher precedence" than :. I like @ but if I read Docutils correctly (#9062 (comment)) then we can't use it. Only -._+ can be used.
I think your first suggestion with :external+python:py:class: is the best. Just for the visual picture, the full set of variations are then

  • :external+python:py:class:, specific inventory, specific domain,
  • :external:py:class:, all inventories, specific domain,
  • :external+python:class:, specific inventory, default domain,
  • :external:class:, all inventories, default domain.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The PR is now updated with this new syntax.

@astrojuanlu
Copy link
Contributor

Would be cool to get this in for 4.4.0! 🚀

@jakobandersen
Copy link
Contributor Author

Would be cool to get this in for 4.4.0! rocket

Indeed, though I think @tk0miya is planning to release it this weekend, so not sure if this is enough time.

Copy link
Member

@tk0miya tk0miya left a comment

Choose a reason for hiding this comment

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

LGTM with nits.

My comments are minor refactoring ideas. So it's okay to fix them after merging. What do you think?

sphinx/ext/intersphinx.py Outdated Show resolved Hide resolved
sphinx/ext/intersphinx.py Outdated Show resolved Hide resolved
@tk0miya tk0miya modified the milestones: 4.5.0, 4.4.0 Jan 15, 2022
@tk0miya
Copy link
Member

tk0miya commented Jan 15, 2022

I'll make a new package tomorrow night in JST (maybe 20hours later from now). So it's not too late to merge this.

@tk0miya tk0miya merged commit fc428ad into sphinx-doc:4.x Jan 16, 2022
@tk0miya
Copy link
Member

tk0miya commented Jan 16, 2022

Merging now. Thank you for your great work!

@jakobandersen jakobandersen deleted the intersphinx_role branch January 22, 2022 12:23
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 22, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
extensions:intersphinx type:enhancement enhance or introduce a new feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants