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

feat: allow admonitions to be rendered #242

Merged
merged 4 commits into from
Jun 30, 2022

Conversation

miketheman
Copy link
Member

@miketheman miketheman commented Jun 25, 2022

Fixes #132
Relates to pypi/warehouse#8300

Important commit: 9277f4a

Signed-off-by: Mike Fiedler miketheman@gmail.com

Create failing examples to test desired behavior.

Signed-off-by: Mike Fiedler <miketheman@gmail.com>
docutils' admonitions renders the blocks using `class` attributes,
which can then be styled via CSS.
Allow `class` attributes to pass through the cleaner.

Signed-off-by: Mike Fiedler <miketheman@gmail.com>
With the allowance that `class` may now be passed for `div`,
update fixtures to conform to the new directive.

Signed-off-by: Mike Fiedler <miketheman@gmail.com>
@miketheman miketheman changed the title test: add test fixtures for admonitions feat: allow admonitions to be rendered Jun 25, 2022
@miketheman
Copy link
Member Author

I didn't find a simpler way to allow a subset of of the class attributes through, so if there's a better way, happy to adapt.

@miketheman
Copy link
Member Author

I played around with bleach's behavior to pass a function instead of string values for ALLOWED_ATTRIBUTES, but didn't come up with anything particularly satisfying.

diff --git a/readme_renderer/clean.py b/readme_renderer/clean.py
index 332dd5b..bba5889 100644
--- a/readme_renderer/clean.py
+++ b/readme_renderer/clean.py
@@ -22,6 +22,37 @@ import bleach.linkifier
 import bleach.sanitizer
 
 
+def _allow_admonitions(tag: str, name: str, value: str) -> bool:
+    """Helper function to allow a subset of admonition-focused `class` attributes"""
+    # Preserves existing behavior, wildcard is overridden
+    if name in ("id", "align"):
+        return True
+
+    admonition_values = (
+        "attention",
+        "caution",
+        "danger",
+        "error",
+        "hint",
+        "important",
+        "note",
+        "tip",
+        "warning",
+        "admonition",
+        "first",
+        "last"
+    )
+
+    if name == 'class' and all(
+        val.startswith(admonition_values)
+        for val
+        in value.split()
+    ):
+        return True
+
+    return False
+
+
 ALLOWED_TAGS = [
     # Bleach Defaults
     "a", "abbr", "acronym", "b", "blockquote", "code", "em", "i", "li", "ol",
@@ -47,7 +78,7 @@ ALLOWED_ATTRIBUTES = {
     "span": ["class"],
     "th": ["align"],
     "td": ["align"],
-    "div": ["align"],
+    "div": _allow_admonitions,
     "h1": ["align"],
     "h2": ["align"],
     "h3": ["align"],
@@ -55,7 +86,7 @@ ALLOWED_ATTRIBUTES = {
     "h5": ["align"],
     "h6": ["align"],
     "code": ["class"],
-    "p": ["align"],
+    "p": _allow_admonitions,
     "ol": ["start"],
     "input": ["type", "checked", "disabled"],
 }

This would allow me to revert any changes to existing fixtures and remove the `class="section=..." values, preserving the existing behavior, but at the cost of this new function.

@miketheman
Copy link
Member Author

@di any thoughts on this?

@di di merged commit 30b5f9a into pypa:main Jun 30, 2022
@miketheman miketheman deleted the miketheman/admonitions branch June 30, 2022 21:02
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.

Support standard rst admonitions
2 participants