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

Override generation of id & href attributes for API documentation #1208

Merged
merged 3 commits into from Apr 25, 2023

Conversation

bheberlein
Copy link
Contributor

This will replace dots with underscores in qualified names of Python modules & functions (e.g. mypackage.mymodule.myfunc) during translation to HTML, allowing ScrollSpy to identify documented API members as navigation targets.

Addresses #1207 and fixes #1026.

@bheberlein
Copy link
Contributor Author

Looks like some more work is needed here. It will replace dots in href attributes of links, which is a pretty big issue that I should have foreseen. I'll see if I can come up with a some edits that will modify attributes in a more targeted way.

This will replace dots with underscores in qualified names of Python modules & functions (e.g. `mypackage.mymodule.myfunc`), allowing ScrollSpy to identify these tags as navigation targets.

Addresses pydata#1207 and fixes pydata#1026
@choldgraf
Copy link
Collaborator

Just a quick thought - this seems like a valuable improvement - though is this not something that should be improved at the autodoc level instead of at the theme level? It feels like something that any HTML theme would benefit from, rather than unique to the pydata theme, right?

@bheberlein
Copy link
Contributor Author

@choldgraf yes, I think it's a fair point. Perhaps the right way to proceed is to open an issue with Sphinx.

@drammock
Copy link
Collaborator

closing because the proposed solution has migrated upstream.

@drammock drammock closed this Mar 21, 2023
@12rambau
Copy link
Collaborator

I agree this fix deserves to be send upstream but in the meantime @bheberlein offers a workaround that could solve the issue we face without waiting for autodoc to change (I think they are a bit overwelmed on Sphinx side). So untill this is solved (sphinx-doc/sphinx#11208) what about integrating this solution ?

@drammock
Copy link
Collaborator

So untill this is solved (sphinx-doc/sphinx#11208) what about integrating this solution ?

OK by me, it's been more than a month with no reply on the upstream issue. Please add # TODO comments in the source that link to the upstream issue and say when it's OK to remove the functions, so that periodically when people edit that file, they can do a quick check to see if it's still needed.

@12rambau I leave it to you to figure out if this should go in before or after #1271

@drammock drammock reopened this Mar 28, 2023
@12rambau
Copy link
Collaborator

it can be done after as the translator.py file was already existing, it will be a small conflict.

Copy link
Collaborator

@12rambau 12rambau left a comment

Choose a reason for hiding this comment

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

@bheberlein I've merged the latest refactoring in your branch. The API page seems to work as expected, solving the issue mentioned in the PR.

Before we merge it could you:

  • reference the issue in Sphinx so that our future self know when to remove this workaround
  • apply the linter on the branch

Copy link
Collaborator

@12rambau 12rambau left a comment

Choose a reason for hiding this comment

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

Thank you very much for updating your PR. I really like the solution you find, all good from my side

@12rambau 12rambau merged commit 1522b84 into pydata:main Apr 25, 2023
14 checks passed
@drammock drammock mentioned this pull request Sep 15, 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.

Scrollspy is broken for API page anchors
4 participants