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

Avoid over-linking #1293 #1294

Merged
merged 6 commits into from
Mar 27, 2024
Merged

Conversation

nedbat
Copy link
Member

@nedbat nedbat commented Mar 20, 2024

Summarizing and codifying the advice discussed in python/docs-community#52


📚 Documentation preview 📚: https://cpython-devguide--1294.org.readthedocs.build/

@nedbat nedbat changed the title Avoid over-linking Avoid over-linking #1293 Mar 20, 2024
Copy link
Collaborator

@willingc willingc left a comment

Choose a reason for hiding this comment

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

Thanks Ned!

documentation/style-guide.rst Outdated Show resolved Hide resolved
documentation/style-guide.rst Outdated Show resolved Hide resolved
documentation/style-guide.rst Outdated Show resolved Hide resolved
documentation/style-guide.rst Outdated Show resolved Hide resolved
@nedbat nedbat force-pushed the nedbat/avoid-overlinking branch from 5df7988 to 8fa2d3a Compare March 20, 2024 16:47
@nedbat nedbat force-pushed the nedbat/avoid-overlinking branch 2 times, most recently from 4ac3e45 to ba8530b Compare March 20, 2024 17:52
@nedbat
Copy link
Member Author

nedbat commented Mar 20, 2024

Changes made, and I added another rule: no links in section titles.

@nedbat nedbat force-pushed the nedbat/avoid-overlinking branch 3 times, most recently from fac2e66 to 0a5704b Compare March 20, 2024 18:43
@nedbat nedbat force-pushed the nedbat/avoid-overlinking branch from 0a5704b to 60426a1 Compare March 20, 2024 18:47
documentation/markup.rst Outdated Show resolved Hide resolved
``:role:`~!hidden.visible``` makes more semantic sense, but it causes
a warning as Sphinx tries to look up the reference ``!hidden.visible``
which does not exist. The shorter form ``:role:`!visible`` renders as
desired and will build successfully.
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure this should be included here. The quick reference lists commonly used markup, and what's being described here is not very common.

I would move this paragraph in the linked "roles" section, where the nuances of using ! and ~ are covered in more detail.

Copy link
Member Author

Choose a reason for hiding this comment

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

It took a few developers a while to work out what happened when trying to combine ~!, so I think it will be helpful to leave here.

Copy link
Member

Choose a reason for hiding this comment

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

What I'm thinking is:

  • the quick reference table is meant mostly for readers that already know about the existence and functioning of those constructs, but don't quite remember the syntax
  • all further details and explanations shouldn't (and aren't) covered here, but in the sections linked in the third column
  • if users look at the "roles" section (where ~ and ! are explained) for ways to combine ~/! they won't find it there, unless you duplicate the note there
  • the wording of roles w/o link, short is not too clear, since short doesn't mirror only last part (understandably, due to lack of space)
  • a reader might be wondering how to combine ~ and ! together, but :role:`!visible` doesn't seem an obvious answer to the question (unless they keep scrolling and read the note), especially since it doesn't have any apparent difference from :role:`!target`
  • the consequences of combining ~ and ! are a Sphinx implementation detail that shouldn't matter to the reader, so I would omit them from the note

My suggestion is to add:

* Combining ``~`` and ``!`` (e.g. ``:meth:`~!Queue.Queue.get```) is not supported.
  You can obtain the same result by simply using `!` and the last component of
  the target (e.g. ``:meth:`!get```).

in the "roles" section, after the bullet points that already explain ~ and ! and use Queue.Queue.get as an example.

The entry in the table could be removed altogether, since it's not a very common construct (trusting that the reader will follow the link looking for more information). If you prefer to keep it, the table should be updated by:

  • widening the first column and using roles w/o link, only last part to mirror the text of roles w/ only last part
  • reordering the entries so that roles w/o link comes first, roles w/ only last part next, and roles w/o link, only last part just after, so that the use of !visible is more evident.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@nedbat Let me take some time to read @ezio-melotti's comment. Ezio makes some good points especially considering the original intent of the chart. There needs to be a new doc contributor cheat sheet (that is useful to current contributors) to guide folks on many of these markup constructs.

Copy link
Member Author

Choose a reason for hiding this comment

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

@ezio-melotti I like your suggestion, and have made it in the latest commit.

documentation/style-guide.rst Outdated Show resolved Hide resolved
documentation/style-guide.rst Outdated Show resolved Hide resolved
@ezio-melotti ezio-melotti linked an issue Mar 21, 2024 that may be closed by this pull request
documentation/markup.rst Outdated Show resolved Hide resolved
documentation/style-guide.rst Show resolved Hide resolved
Co-authored-by: Hugo van Kemenade <[email protected]>
@hugovk hugovk merged commit ea7063a into python:main Mar 27, 2024
4 checks passed
@nedbat nedbat deleted the nedbat/avoid-overlinking branch March 27, 2024 13:06
@hugovk
Copy link
Member

hugovk commented Mar 27, 2024

Thanks!

@willingc
Copy link
Collaborator

Thanks @nedbat

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.

Document only adding links once per paragraph
6 participants