Infra: Use an option instead of argument in banner directives #3765
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Sphinx presents a clear error message if you have any content without whitespace after these banner directive's options: With this change added in... --- a/peps/pep-0560.rst
+++ b/peps/pep-0560.rst
@@ -12,6 +12,7 @@ Resolution: https://mail.python.org/pipermail/python-dev/2017-December/151038.ht
.. canonical-doc::
:related: :external+python:meth:`object.__class_getitem__` and
:external+python:meth:`object.__mro_entries__`
+ Content
Abstract
========Running a build results in: While this behaviour is mentioned in the commit message, I figured it'd be useful to have this error message mentioned here to provide additional context. This is a good place to also mention a caveat(?) with this approach: If there's an additional space (i.e. indentation of |
hugovk
changed the title
When a reST directive has `has_content` set to `True`, optional
arguments and `final_argument_whitespace` set to True, the presence of a
newline at the start of the directive is significant in communicating
whether specific markup is treated as the argument or content.
.. banner-1:: This is an argument
This is still the argument.
This is content.
.. banner-2::
This is still the argument.
This is content.
.. banner-3::
This is content.
This is more content.
In the above example, `banner-2` and `banner-3` are very similar and
only different in the presence of a newline. This is a subtle failure
mode where written content can end up being silently ignored by the reST
directive due to how arguments vs contents are parsed.
Instead of accommodating for this behaviour by adding additional
complexity to the directive being used, this change stops trying to mix
multiline arguments and content in a single directive by using explicit
options instead. This requires more verbosity but also eliminates this
failure mode entirely.
.. banner-1::
:option: This is the option.
This is still the option.
.. banner-2::
This is content.
This is still content.
.. banner-3::
This is content.
This is still content.
With this change, presence of a newline at the start of the directive is
not determining if specific markup is treated as the argument or
content. This is instead communicated by the presence of the option
syntax which is more explicit and presents proper errors when the syntax
is incorrect.
pradyunsg
force-pushed
the
tweak-link-content-handling
branch
from
13144fb
to
a8a4bfb
Compare
AA-Turner
reviewed
May 6, 2024
pep_sphinx_extensions/pep_processor/parsing/pep_banner_directive.py
Outdated
Show resolved
Hide resolved
pep_sphinx_extensions/pep_processor/parsing/pep_banner_directive.py
Outdated
Show resolved
Hide resolved
Co-authored-by: Adam Turner <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
(context for PEP authors: This PR will not change how a specific PEP is presented or how it would be rendered -- if you're not interested in PEP infra changes, please feel welcome to unsubscribe)
This is an "infra" change, tweaking the specific reST syntax structures being used in the directives used to provide document-wide deprecations or other links. See also #2719, which tracks the wider effort around this.
For the problem that motivated this change, see #3682 and #3756. See the commit message for the details of what the problem's cause was and how this change deals with that.
📚 Documentation preview 📚: https://pep-previews--3765.org.readthedocs.build/