Rewrite emphasis handling - #1632
Conversation
- This is a complete rewrite of how emphasis handling is done. - Drop use of multiple regex patterns in run in multiple passes and instead evaluate delimiters, nested or otherwise, and build up HTML elements. - Try to consume tokens as much as possible until a full element is constructed (with children if any). - If an outer set of tokens cannot be resolved, but one or more sub tokens can, render the first sub token span and cache the remaining ones for subsequent reentry and render those until the cache is exhausted. - Two tests results were updated to match new behavior.
|
This is purposely a draft and is being made available for testing. More tests should be written, etc. For review. The proposal is to first release this in Pymdown Extensions, and once vetted in the real world, make it the default approach, but I will let Python Markdown make the decision to release it before then if desired. |
|
Awesome! I haven't had an opportunity to fully review the code, but the idea seems sound. The one thing that gives me pause is that we are reducing the number of inline processors to run. In itself, that is not a problem, but it may be that some third-party extensions are expecting to inject their processors between a few of the now-combined processors. I'm not certain if that will cause issues for those extensions or not. Therefore, my inclination is to consider this a backward incompatible change. Presumably the release notes should include a warning for third-party extension devs. I don't think there is any way we can gracefully make these changes through deprecations. Interestingly, the change this relies on already was pushing the next release into at least a minor release. Adding this to the same release would be ideal from that perspective. But I also see the desire to test more thoroughly. I'm undecided about it for now. |
|
Yep, I'm under no illusion that this is not a big change. While I think that if people notice anything, it will be minor differences, or actually have it handle certain nesting more like other parsers, it is a complete and total rewrite from the ground up, and that does carry some risk until fully vetted.
Totally understood. I wouldn't design it differently, but I get why this is a flag that it's a bigger change and may have other surprising impacts outside of what becomes emphasis and what does not. I've fiddled with the regex for a while now to try to make case X better, or make case Y more performant, but I think I finally just reached the limit of what could be done with multiple regex patterns applied in sequence, and finally did what I always knew was the answer. But to be honest, without the recent fix in Python Markdown, I couldn't implement this. Personally, I'm releasing it as a "breaking" change in Pymdown Extensions, despite expecting/hoping most people won't notice differences. I'm okay with whatever Python Markdown wants to do, and I'm happy to clean it up for merge if we want to push it out early rather than later, but I'll leave it in the draft state unless I hear a push to get it in. At the very least, it is available to be freely tested and put through its paces, and I think that is the most important thing; we now have something that can be evaluated for as long as we feel is necessary. |
This is a prevented measure to ensure the processor is always in a good state. This situation has never been observed, but if it did occur, this would allow the processor to reset its state and continue properly. Only extensions have a way to reset, processors don't. Nor do they have a way to detect when a reset would be needed. - Add a current time for each Markdown run. - Have DelimiterProcessor compare the each run time, and if it has changed, perform a reset of the stack.
|
I've cleaned things up a bit and fixed failing CI stuff. Over the next couple of days, I'll bring over some more tests to make sure we have good coverage and checks that assert expected logic for more advanced nesting cases. The only new addition is that I added a Inline processors don't have a reset hook, that requires an actual, registered extension, but since we do preserve a state between calls so that we can efficiently serve up already found emphasis, it is possible if the Markdown object were to crash for any reason, the processor could be left in a bad state on a rerun. So adding this timestamp allows us to see that we are in a new run, so we can reset the state if it hasn't already been done. This is a preventive measure, and I haven't actually encountered this case, but it is certainly a plausible scenario. |
| Called once upon creation of a class instance. Should be called manually between calls | ||
| to [`Markdown.convert`][markdown.Markdown.convert]. | ||
| """ | ||
| self.last_run = time.time() |
There was a problem hiding this comment.
It's not clear to me why we need this. If I understand correctly, this initiates a reset of the extension once for each run of md.convert. Why not just have md.reset() accomplish that (by registering the extension)?
There was a problem hiding this comment.
It's an inline processor, I wasn't sure the best way to handle it as it isn't wrapped in an extension, though I guess we could...I guess we could send the inline processor through register directly, but that didn't seem proper 🤷🏻
Description
AI Assistance Disclosure
Checklist