miyuchina / mistletoe

A fast, extensible and spec-compliant Markdown parser in pure Python.
MIT License
818 stars 118 forks source link

feat: add parent attribute (#71) #206

Closed pbodnar closed 8 months ago

pbodnar commented 9 months ago

This resolves #71 and is a continuation / replacement for PR #72.

coveralls commented 9 months ago

Coverage Status

coverage: 94.198% (+0.03%) from 94.166% when pulling 6744856ab739d05c648e64068aa085941f9a8c3c on issue-71-add-parent-attribute into 823b536210fcc9629d650a959ea06d414e9aa81e on master.

pbodnar commented 9 months ago

I'm hopefully choosing the simplest solution for introducing this feature. By making both the children and parent attribute a @property, we can extend the existing code and centralize the solution without much refactoring - although the API won't remain fully backwards compatible (hasattr(...) needs to be replaced by ... is not None etc.).

An alternative approach would possibly be to pass the parent to every token constructor up to the Token class, but if I'm not mistaken, this would require much more changes - which would newly require calling super().__init__(...) in all the token classes. And I'm not sure it is a good time for doing that now.

Another thing is the performance penalty - running the benchmark (python test/benchmark.py mistletoe or running it via mprof to also see memory usage) shows me that mistletoe gets 4% slower and uses +13% memory when recording the children's parent. These are not great results, OTOH 1) it shouldn't be of such a big deal nowadays, 2) it cannot be probably implemented in a more efficient way (or can it?), 3) all the other competing tools (markdown, commonmark and its "de-facto replacement" markdown-it-py) seem to record token's parent (with no option to turn this off).

All in all, if someone (like @anderskaplan? ;)) doesn't propose a better solution, I would finish this PR (adding some docs and updating the benchmark results (yes, we should automate running those benchmarks...)) and merge it to the master branch. I think this would nicely complement the recent PR #188...

pbodnar commented 8 months ago

I've decided to merge this into the master and update the benchmark results later on (there is more to update).

Here are the COMPATIBILITY REMARKS:

vallentin commented 6 months ago

Hey, sorry for not coming back to this. But since this has been merged, then I guess we can close my PR #72? :)

pbodnar commented 6 months ago

@vallentin, you are right, thanks for the reminder. 👍