Skip to content

PrettifyTreeprocessor AttributeError #1261

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

Closed
fourpoints opened this issue May 24, 2022 · 6 comments
Closed

PrettifyTreeprocessor AttributeError #1261

fourpoints opened this issue May 24, 2022 · 6 comments
Labels
bug Bug report. confirmed Confirmed bug report or approved feature request. core Related to the core parser code.
Milestone

Comments

@fourpoints
Copy link
Contributor

fourpoints commented May 24, 2022

This code section near the bottom of markdown.treeprocessors raises AttributeError if pre[0].text is None.

# Clean up extra empty lines at end of code blocks.
pres = root.iter('pre')
for pre in pres:
    if len(pre) and pre[0].tag == 'code':
        pre[0].text = util.AtomicString(pre[0].text.rstrip() + '\n')

The check should be if len(pre) and pre[0].tag == 'code' and pre[0].text is not None or isinstance(pre[0].text, str), but that may be too strict.


I encountered this with a custom Block/Inline processor. The band-aid fix was just to ensure these elements had text.

for pre in root.iter("pre"):
    if pre[0].text is None:
        pre[0].text = ""
@fourpoints fourpoints changed the title PrettifyTreeprocessor TypeError PrettifyTreeprocessor AttributeError May 25, 2022
@waylan
Copy link
Member

waylan commented May 25, 2022

Based on the information you provided, I don't understand how that results in an error. All we are doing is resigning pre[0].text to a new value, which is exactly the same thing you are doing in your band-aid. Could you please point to where this code is so I can better understand it in context?

@waylan
Copy link
Member

waylan commented May 25, 2022

Oh, I got it. pre[0].text.rstrip() raises an error if pre[0].text is None.

I believe this has not been an issue thus far as internally we always assign a string value to the text attribute of an element. The question is: do we want to require all extensions to do the same or not? If not, then we should apply the suggested fix. If so, then we can leave it alone.

Finally, the relevant code is here for future reference.

@waylan waylan added the needs-decision A decision needs to be made regarding request. label May 25, 2022
@fourpoints
Copy link
Contributor Author

By default ET.Element(tag).text is None. But you could add a requirement that it is a string. Thanks for linking to the code.

@waylan
Copy link
Member

waylan commented May 25, 2022

I was curious how the serializer handles a None value so I checked the code:

if text:
if tag.lower() in ["script", "style"]:
write(text)
else:
write(_escape_cdata(text))
for e in elem:
_serialize_html(write, e, format)
if tag.lower() not in HTML_EMPTY:
write("</" + tag + ">")

When text is None , if text evaluates to False and the serializer simply skips writing out the text content and moves on to child elements (if any) and the closing tag. It makes sense for an element with text of None to simply be empty (no different that text being an empty string). Therefore, there is no reason to require an element's text attribute to be of type str.

Finally, the question remains whether the prettifier needs to take any action on an empty code block. In other words, do we want

<pre><code></code></pre>

or

<pre><code>
</code></pre>

If the former, then the suggested fix will work fine. If the latter, then we need a slightly more sophisticated fix. Checking Balemark (compare this with this) is looks like the first option is acceptable.

Given the above, a PR is welcome adding and pre[0].text is not None to the check.

@waylan waylan added bug Bug report. core Related to the core parser code. confirmed Confirmed bug report or approved feature request. and removed needs-decision A decision needs to be made regarding request. labels May 25, 2022
@waylan waylan added this to the Version 3.4 milestone May 25, 2022
@fourpoints
Copy link
Contributor Author

Thanks, I'll make a PR later :)

@fourpoints
Copy link
Contributor Author

Thanks for your help! 👍

Minor note, this also affects non-empty blocks, i.e. blocks with children but no text, such as <pre><code><span>x</span></code></pre>. But I don't think that changes anything here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Bug report. confirmed Confirmed bug report or approved feature request. core Related to the core parser code.
Projects
None yet
Development

No branches or pull requests

2 participants