esbonio icon indicating copy to clipboard operation
esbonio copied to clipboard

Published 20 hours ago • verified publisher icon swyddfa

Intersphinx: support external/external+ roles

Open stsewd opened this issue 2 years ago • 4 comments

  • 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 https://github.com/swyddfa/esbonio/issues/464

stsewd avatar Dec 10 '22 19:12 stsewd

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 avatar Dec 13 '22 19:12 alcarney

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 :smile:

alcarney avatar Jan 10 '23 23:01 alcarney

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

stsewd avatar Jan 12 '23 02:01 stsewd

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

alcarney avatar Jan 12 '23 22:01 alcarney

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 avatar Jul 19 '24 18:07 alcarney

No worries, thanks for your work on esbonio!

stsewd avatar Jul 23 '24 02:07 stsewd