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

Distinguish name of extension, name of sample target file and name of… #79

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

jd41
Copy link

@jd41 jd41 commented Jun 12, 2020

… sample target section

I think the documentation is somewhat clearer if the role, the example target file and the example target section aren't all named "hoverxref". I am a Sphinx/RST beginner, currently hunting down why my hoverxref example doesn't work. Having made these changes, I can confirm that having a section "hoverxref_section" within a file "hoverxref_file" should be enough to create a findable target "hoverxref_file:hoverxref_section". It was less clear before. A new user can now easily confirm this by grepping for "hoverxref_section".

Update: Sorry, I mixed up some words in the commit message description! Please refer to this PR description.

jd41 added 2 commits June 12, 2020 22:38
… sample target section

I think the documentation is somewhat clearer if the role, the example file and the example target section within that role aren't all named "hoverxref". I am a Sphinx/RST beginner, currently hunting down why my hoverxref example doesn't work. Having made these changes, I can confirm that having a section "hoverxref_section" within a file "hoverxref_file" should be enough to create a findable target "hoverxref_file:hoverxref_target". It was less clear before. A new user can now easily confirm this by grepping for "hoverxref_target".
Copy link
Member

@humitos humitos left a comment

Choose a reason for hiding this comment

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

This is a good point! I want to think more about the proposed names (hoverxref_file and hoverxref_section). Maybe calling them example.rst and title. The problem with title is the modal popup will show "Title" as the title of the modal instead of "hoverxref" and won't look great.

On the other hand, the extension uses basic Sphinx :ref: role references and targets; it's not something specific from hoverxref. So, it may be good to link to the :ref: role documentation somewhere at the very beginning of the documentation instead of changing these names.

@jd41
Copy link
Author

jd41 commented Aug 30, 2020

I actually disagree that "Title" looks worse than "hoverxref". What about "example_file.rst", "Example section title", and "example_label"? The important thing is just that they are all different and unique (i.e. these terms don't appear anywhere else in a relevant source code) - if we spend more time discussing them, I'm gonna call bikeshedding =)

I propose "example_label" because the result of my debugging two months ago was that I hadn't understood the need to use the autosectionlabel extension so that the section titles automatically get turned into labels. I think that using this extension in the hoverxref example is a tripwire for a new user as well, so I'd also consider manually labeling the target section here instead. Then the documentation gets closer to a minimal working example of hoverxref. Alternatively, one could point out the usage of autosectionlabel explicitly.

Update: I forgot that after including autosectionlabel, I still had to figure out that I also need to set autosectionlabel_prefix_document = True to get this label style to work. So that's another small tripwire.

jd41 added a commit to jd41/sphinx-hoverxref that referenced this pull request Sep 20, 2020
Calling the sample project "sphinx-hoverxref" might be a bit confusing, and it should be made clear that "latest" refers to the documentation's version, not somehow the hoverxref version. See also PR readthedocs#79.

I am not sure whether changing "latest" to "your-version" is an improvement, actually, feel free to revert that part if you disagree... the change is not too important anyway.
@jd41 jd41 requested a review from humitos November 2, 2020 09:25
@jd41
Copy link
Author

jd41 commented Nov 2, 2020

Hi! Sorry, I didn't understand I should have clicked on "re-request review".

Would you like to decide on how to call them for now, and merge that thing? I feel that either the current PR or your suggestion or my suggestion in the last message improves on the status quo. If an even better solution is possible, of course I won't object if you change something again soon =)

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.

2 participants