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

Move from overridden domains to a custom role #14

Closed
agjohnson opened this issue Aug 21, 2019 · 5 comments · Fixed by #43
Closed

Move from overridden domains to a custom role #14

agjohnson opened this issue Aug 21, 2019 · 5 comments · Fixed by #43
Labels
Needed: design decision A core team decision is required

Comments

@agjohnson
Copy link

Instead of using a custom role, this extension is using overrides on domains, which is going to introduce problems with reference resolution as Sphinx has multiple Python and Standard domains.

Couldn't this instead implement our own custom role, instead of repurposing XRefRole and having to catch reference resolution in every domain?
https://github.com/readthedocs/sphinx-hoverxref/blob/master/hoverxref/extension.py#L176-L184

@humitos
Copy link
Member

humitos commented Aug 21, 2019

It's defining a custom role as well, :hoverxref:. The thing with that is that you have to re-write all your documentation to make the tooltip appear on cross-references.

@agjohnson
Copy link
Author

agjohnson commented Aug 21, 2019

But it might be possible to patch the standard roles used instead of replacing the domains.

Replacing the domains will eventually be fragile, and will likely cause linking issues.

@humitos
Copy link
Member

humitos commented Aug 21, 2019

If we can get the domain used to produce/resolve a node, we can just connect to doctree-resolved event and traverse the nodes but I'm not sure that's possible. We need the "logic to resolve the reference" from the domain.

It may be possible, but I'm not seeing it clearly. Do you have something in mind?

@agjohnson
Copy link
Author

agjohnson commented Aug 22, 2019

Not yet, this will require more experimentation. This is likely getting into a solution that might hack up Sphinx at a deeper level. But, as often is the case here, a lightweight hack in Sphinx internals can actually be less fragile than hoping the domains can be overridden without issues.

We can alter the doctree through capturing an event, but if you don't have the correct information on the resolved ref node, this might not be a good avenue.

@humitos
Copy link
Member

humitos commented Aug 23, 2019

But, as often is the case here, a lightweight hack in Sphinx internals can actually be less fragile than hoping the domains can be overridden without issues

Yeah. Overriding the Domains is not an elegant solution, but I will only fail if there is another extension that also overrides the same domain.

Also, I think that there is more research that can be done about how override=True is implemented and see if we can save all the domains and base our class on those ones (inherit) instead of overriding them directly.

I'd like to find a way to extend an already defined domain, but I wasn't able find that path yet.

@humitos humitos added the Needed: design decision A core team decision is required label Aug 23, 2019
humitos added a commit that referenced this issue Apr 5, 2020
Follow the same pattern for translators (#42) for domains.

Closes #14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needed: design decision A core team decision is required
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants