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

Infra: add newlines so admonition text is shown #3756

Merged
merged 2 commits into from
Apr 14, 2024

Conversation

And remove text duplicating that inserted by the template.
Copy link
Member

@malemburg malemburg left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@Yhg1s Yhg1s left a comment

Choose a reason for hiding this comment

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

The need for a newline feels a bit like a footgun, is there a way to address that, or have it show up as a lint warning?

@hugovk
Copy link
Member Author

hugovk commented Apr 14, 2024

Will check into that before doing the next batch, it might be something to do with how these custom directives set admonition_pre_template/admonition_pre_text: https://github.com/python/peps/pull/3682/files#diff-9289f0259c1905b9816aa119d9e3e457cadd059c916a83de500732f15ae8109dR130

@hugovk hugovk merged commit 8ada2c9 into python:main Apr 14, 2024
6 checks passed
@hugovk hugovk deleted the infra-update-admonitions branch April 14, 2024 20:09
@encukou
Copy link
Member

encukou commented Apr 15, 2024

AFAIK that's normal ReST syntax: the blank line separates a directive's arguments/options from its content.
Since admonitions have final_argument_whitespace = True, this block is treated as (a continuation of) the argument, that is, link_content.

I don't see a good way to disable the argument-continuation/option block, short of parsing the entire directive manually (using self.block_text in run).

Perhaps it'd be best to warn/error on suspicious cases:

  • self.content is empty but link_content is not
  • self.content is empty and link_content is multi-line
  • there's some link_content but the template doesn't expect it

@pradyunsg
Copy link
Member

I wrote a long blurb here and realised it would be easier to just draft up the change -- so, filed #3765 as a potential way forward here.

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.

5 participants