Infra: Use an option instead of argument in banner directives - #3765
Draft
pradyunsg wants to merge 2 commits into
Draft
pradyunsg wants to merge 2 commits into
pradyunsg wants to merge 2 commits into
Conversation
Copy link
Copy Markdown
Member
Sorry, something went wrong.
Copy link
Copy Markdown
Member
Author
|
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: Sphinx parallel build error:
docutils.utils.SystemMessage: /Users/pradyunsg/Developer/github/peps/peps/pep-0560.rst:12: (ERROR/3) Error in "canonical-doc" directive:
invalid option block.
.. canonical-doc::
:related: :external+python:meth:`object.__class_getitem__` and
:external+python:meth:`object.__mro_entries__`
Content
Sphinx exited with exit code: 2
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 Content is greater than that of :related:, it'll be considered a part of the option's value). I don't think it's an equivalent failure mode to what we have right now but also... I don't see any way to avoid this behaviour from docutils/reST. |
Sorry, something went wrong.
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
April 28, 2024 18:50
AA-Turner
reviewed
May 6, 2024
AA-Turner
left a comment
Copy link
Copy Markdown
Member
There was a problem hiding this comment.
Seems sensible.
A
Sorry, something went wrong.
Comment thread
pep_sphinx_extensions/pep_processor/parsing/pep_banner_directive.py
Outdated
Comment thread
pep_sphinx_extensions/pep_processor/parsing/pep_banner_directive.py
Outdated
Co-authored-by: Adam Turner <9087854+AA-Turner@users.noreply.github.com>
Copy link
Copy Markdown
|
All commit authors signed the Contributor License Agreement. |
Sorry, something went wrong.
This branch has not been deployed
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Reviewers
2 participants
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/