Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings
This repository was archived by the owner on Apr 9, 2025. It is now read-only.

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

Open
jd41 wants to merge 2 commits into readthedocs:main
base: main
Choose a base branch
Loading
from jd41:differentiate-names

Conversation

@jd41
Copy link

@jd41 jd41 commented Jun 12, 2020
edited
Loading

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

funkyfuture reacted with thumbs up emoji
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.

Copy link
Author

jd41 commented Aug 30, 2020
edited
Loading

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
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 subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@humitos humitos Awaiting requested review from humitos

At least 1 approving review is required to merge this pull request.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

AltStyle によって変換されたページ (->オリジナル) /