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: support external/external+ roles #514

Closed
wants to merge 6 commits into from

Conversation

stsewd
Copy link
Contributor

@stsewd stsewd commented Dec 10, 2022

  • The current regex for roles wasn't taking into consideration :foo:bar:baz: roles (roles with more than two :)
  • Add support for external roles into the InterSphinx feature (maybe subclassing? InterSphinxExternalRoles)
  • Missing tests and fixing current ones
  • The documentation/hover feature requires passing a static dictionary to the Role class, but maybe it should be part of the RoleLanguageFeature class, so we can do that dynamically, this is useful since the documentation for these new external roles is basically the same as normal roles :external:doc: == :doc:. Of course, this should already be possible with add_documentation, but having it in RoleLanguageFeature encapsulates all the logic there.

Closes #464

@alcarney
Copy link
Member

Thanks for looking at this!

(maybe subclassing? InterSphinxExternalRoles)

I think it's fine to keep everything in InterSphinx

The documentation/hover feature requires passing a static dictionary to the Role class, but maybe it should be part of the RoleLanguageFeature class

Maybe we should have both options, it definitely makes sense to be able to do this dynamically in the intersphinx case but I think it also makes sense to just call add_documentation if someone wants to provide some simple docs for roles they added in their project's conf.py

@alcarney alcarney mentioned this pull request Dec 27, 2022
@alcarney
Copy link
Member

Hey, just thought I'd check in and see if you needed anything from me on this? I hope you haven't been waiting on me without me realising!

Also, would you be ok with me merging #484 ahead of this? Happy to help realign this branch if needed 😄

@stsewd
Copy link
Contributor Author

stsewd commented Jan 12, 2023

@alcarney please go ahead, don't wait for me! I've been busy, so I may be a little slow to complete this PR, the only thing I'm missing is adding tests. Haven't written documentation lately, but I'm using this branch just to make sure nothing breaks.

@alcarney
Copy link
Member

No worries, just wanted to make sure I wasn't going to pull the rug out from under you :)

@alcarney
Copy link
Member

Thanks again for looking at this, sorry that we couldn't get this merged before I started the 1.0 rewrite :/

This was however a useful reference and you'll hopefully be pleased to hear the support for :external: completions is finally coming in #856

@alcarney alcarney closed this Jul 19, 2024
@stsewd stsewd deleted the add-external-role-support branch July 23, 2024 02:29
@stsewd
Copy link
Contributor Author

stsewd commented Jul 23, 2024

No worries, thanks for your work on esbonio!

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.

Support for the new intersphinx syntax (:external: role)
2 participants