Repository navigation
Fix toc crash on a heading with a valueless id attribute - #1706
rahul05ranjan wants to merge 3 commits into
Conversation
The attr_list extension writes bare attributes (``{id}``) with a value
of None. The toc extension's "do not override pre-existing ids" check
only tested key membership, so a heading whose id attribute existed but
carried no value skipped slug generation and later passed the None to
re.sub through unescape(), raising an unhandled TypeError:
markdown.markdown("#d {id}", extensions=[attr_list, toc])
TypeError: expected string or bytes-like object, got 'NoneType'
A valueless id is no id at all, so toc now treats a falsy id like a
missing one and generates a slug for it, exactly as for any other
id-less heading.
Fixes Python-Markdown#1705
Co-authored-by: rahul05ranjan <[email protected]>
|
As per our Contributing Guidelines:
I know you included the following in your comments:
But that doesn't cover everything we ask for in our template. Also, if you can explain everything, then why not explain it yourself instead of dumping AI output and making me read through a bunch of unnecessary stuff? For example, the "What's Wrong" section is completely unnecessary because you already indicated that this "fixes #1705" (the problem has already been defined). The first sentence under "Fix" would have been sufficient (the tests in your patch provide a demonstration, so the example is unnecessary). And the "Verification:" section provides little value once all of the CI checks run here. In the end, one or two sentences handwritten by a human would be much more likely to encourage me to take the time to review this PR. Instead, the entire experience is just annoying to me. As the Contributing Guide notes (emphasis added):
|
The pyspelling lint reads comments and docstrings as plain words, so the "attr_list" name in the new toc.py comment and the "TypeError" mentioned in the new test docstring tripped the checkspelling job.
|
You're right, and I've redone the description: it's now the two sentences that matter, laid out on the PR template you linked, with the template's checkboxes filled in as they apply. The "What's wrong" example and the verification play-by-play are gone. I've also fixed the one failing check — the spelling lint read the new comments as prose and flagged |
There was a problem hiding this comment.
Actually, TypeError is code and should be in a codespan, which the spellchecker will ignore. I didn't check now attr is being used, but if it is code, then the same rule applies. If it is an abbreviation, then we do not use abbreviations, but fully spell out words, which is noted in our style guides in the Contributing Guide.
"TypeError" is code and "attr_list" is the name of an extension, so per the style guides they belong in code spans (which the spellchecker ignores) rather than in .spell-dict. Drop the dictionary entries and wrap the words in code spans in the affected comments and docstrings. Co-authored-by: rahul05ranjan <[email protected]>
|
You're right — the dictionary entries were the wrong tool. Fixed in 821ae37: the |
Description
Fixes #1705. The
attr_listextension writes a bare attribute like{id}with aNonevalue, and thetocextension's "do not override existing ids" check only tested key membership — so a heading with a valuelessidskipped slug generation and later crashed inre.sub. The fix treats a falsyidthe same as a missing one and generates a slug.AI Assistance Disclosure
AI tools were used and I have disclosed which ones, and have fully reviewed and verified their output.
AI tools were used to assist with the following:
Checklist