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: add parent attribute (#71) #206

Merged
merged 1 commit into from
Jan 27, 2024
Merged

Conversation

pbodnar
Copy link
Collaborator

@pbodnar pbodnar commented Dec 17, 2023

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

@coveralls
Copy link

coveralls commented Dec 17, 2023

Coverage Status

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

@pbodnar
Copy link
Collaborator Author

pbodnar commented Dec 17, 2023

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...

At the same time, we also make the `children`
attribute a _property_. This makes it easier for us
to set the `parent` to every child token, without
having to change much of the existing code.
@pbodnar pbodnar force-pushed the issue-71-add-parent-attribute branch from 2148c33 to 6744856 Compare January 20, 2024 16:17
@pbodnar pbodnar changed the title WIP: feat: add parent attribute (#71) feat: add parent attribute (#71) Jan 20, 2024
@pbodnar pbodnar added the breaking-change <https://en.wikipedia.org/wiki/Backward_compatibility> label Jan 20, 2024
@pbodnar pbodnar added this to the 1.4.0 milestone Jan 21, 2024
@pbodnar
Copy link
Collaborator Author

pbodnar commented Jan 27, 2024

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:

  • As the children attribute changed to property, existing code needs to be changed like this:
    -hasattr(token, 'children')
    +token.children is not None
    
    # ...
    
    -'children' in vars(token)
    +token.children is not None
    
    # ...
    
    -getattr(token, 'children', [])
    +token.children or []

@pbodnar pbodnar merged commit 06f2a93 into master Jan 27, 2024
9 checks passed
@vallentin
Copy link
Contributor

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

@pbodnar pbodnar mentioned this pull request Mar 20, 2024
@pbodnar
Copy link
Collaborator Author

pbodnar commented Mar 20, 2024

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking-change <https://en.wikipedia.org/wiki/Backward_compatibility> feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Get parent token
3 participants