sphinx.ext.extlinks: Allow %s in link caption string

[ghost]

Subject: sphinx.ext.extlinks: Allow %s in link caption string

Feature or Bugfix

• Feature

Purpose

• Allow %s in link caption string of the sphinx.ext.extlinks extensions, which allows arbitrary placement of the role content in the link caption.
• LT;DR: We replace prefix + rolecontent with prefix % rolecontent.

Detail

Currently sphinx.ext.extlinks ("extlinks") allows placing the role contents (often a number)
only at the end of the link caption, like so "issue 123". However, sometimes it is needed
to place the number (or text in general) at a different location, e.g. "1. quarter", "xyz house".

This patch changes the behaviour of the second part in the extlinks config. The caption
string now allows %s which gets replaced with the role content (the same substitution
which happens for base URL). Until now this string was called prefix this is changed to
caption. (In fact most of the changes in this diff are renaming prefix to caption.) If
you feel title is a more appropriate name, go ahead and complain.

Example:

conf.py:

extlinks = {'quarter': ('https://example.org/quaters/%s', '%s. quarter')}

index.rst

These are the results of the :quarter:`2`.

Output

These are the results of the 2. quarter.

Note on backwards compatibility:

It is now required that the second entry in the tuple is either None or a string which contains
exactly one %s. If this is not the case, then a warning is printed and we fall back to the
old behaviour, i.e. concatenating the caption label with the role content. This is the same
mechanism as for the base URL. Thus, module a warning this is backwards compatible.
I've filed this merge request against 3.5.x, if you feel this is too much breakage, please
reassign to merge with 3.x.

[ghost]

Is there any interest in picking this up?

[tk0miya]

Sorry for the late response. Good idea. But it would be better to keep compatibility. Could you reconsider the behavior?

[ghost]

Hi @tk0miya

Thanks for coming back to me.

@tk0miya in the review:

The requirement of "%s" must be a breaking change. Do you intend to all of extlinks users should update their configuration after the upgrade? I think it would be better to behaves as prefix if the caption does not contain any "%s".

For an old config the behaviour of Sphinx is the same as previous (i.e. use it as a prefix) except additionally a warning will be printed. In particular, Sphinx generates the same documentation output as previously, so there will be no breakage and no one is forced to upgrade their config to continue building their project. Instead, we gently tip them off to the newer syntax using the warning, so they might update if they fell like it.

This is the same change as was done when upgrading the URL-string to allow a %s (see commit 6b13a28). Maybe have a look at not only the diff but at the resulting file, where you will see that my change (wrt caption) exactly mimics the behaviour wrt full_url. Wouldn't it be strange to let these two parameters warn differently wrt to substitution?

I admit the documentation is strongly worded with a "must have" and "exactly one", if you like, I rephrase that to the more handwavy phrasing used for the URL part (which I didn't touch but has exactly the behaviour now proposed for caption), namely "the target given in the role is substituted in the caption in the place of %s". However, I do not like that style for documentation as it leaves the use in the dark what happens in other cases or whether these other cases are considered "valid".

If you still feel that we should (silently) fall back to the old behaviour instead of printing a warning. Then I'll redo that part and the documentation. Maybe we can add a "Falling back to old prefix behaviour" to the warning text, to make it clear what happened.

[tk0miya]

Thank you for your explanation. Finally, I understand what you're doing. And my last comment was bad. Indeed, the code keeps compatibility.

Now I'm debating about the deprecation step of the configuration. I hesitate the new warning causes CI errors in many projects (because they use -W option usually). So it might be better to support the new format in v4.0 and deprecate (emit a warning) the old one in v5. It would reduce the trouble a bit more. What do you think?

[ghost]

@tk0miya wrote:

Now I'm debating about the deprecation step of the configuration. I hesitate the new warning causes CI errors in many projects (because they use -W option usually). So it might be better to support the new format in v4.0 and deprecate (emit a warning) the old one in v5. It would reduce the trouble a bit more. What do you think?

Reading through the projects Deprecation Policy [0] (I assume this feature hits Sphinx 4.0 or 4.1).
According to the policy:

• Sphinx 4.x will contain a backwards-compatible replica of the function which will raise a RemovedInSphinx60Warning. This is a subclass of PendingDeprecationWarning, i.e. it will not get displayed by default.

• Sphinx 5.x will still contain the backwards-compatible replica, but RemovedInSphinx60Warning will be a subclass of DeprecationWarning then, and gets displayed by default.

• Sphinx 6.0 will remove the feature outright.

To me this looks like a clean and consistent way of getting rid of old cruft while giving users enough time to adapt (Well done!). The most affected ones are arguably developers and CI, which already know the drill.

I updated the code accordingly and took inspiration from sphinx/ext/autodoc/__init__.py regarding the deprecation warning mechanism. Once we agreed on the implementation, I'll squash the fixup commits and order them appropriatly.

[0] https://www.sphinx-doc.org/en/master/internals/release-process.html

[tk0miya]

Thank you for the updates. LGTM!

[tk0miya]

Note: To let users know the deprecation, I'll use logger.warning instead of RemovedInSphinxXXWarning since v5.x. It will cause a crash if users use -W option on their CI. I believe it's helpful to notify the change to them surely.

[tk0miya]

I wrongly merged this into the 3.5.x branch... To fix my mistake up, I reverted and cherry-picked these commits on our repository directly (without force push).

[tk0miya]

Thank you for your contribution!

[ghost]

Thanks to you for merging and reviewing!