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

🐛: Anchor ids in separate renders should not affect each other. #43

Merged
merged 3 commits into from
Sep 15, 2022

Conversation

distractedmosfet
Copy link
Contributor

The anchors plugin remembered used anchor ids per MarkdownIt object for entire lifetime of the object. This means if you use the same MarkdownIt object to render multiple documents then the generated anchor ids will be affected by the order of rendered documents.

This PR makes it clear the known ids after anchors are generated, so separate renders should not affect each other.

This seems like a safe fix to my eyes but I haven't examined these systems in-depth. It does past the single anchors test still.

The anchors plugin remembered used anchor ids per MarkdownIt object for entire lifetime of the object. This means if you use the same MarkdownIt object to render multiple documents then the generated anchor ids will be affected by the order of rendered documents.

This commit makes it clear the known ids after anchors are generated, so separate renders should not affect each other.
@welcome
Copy link

welcome bot commented Jun 2, 2022

Thanks for submitting your first pull request! You are awesome! 🤗

If you haven't done so already, check out EBP's Code of Conduct and our Contributing Guide, as this will greatly help the review process.

Welcome to the EBP community! 🎉

Actually, if I'm just gonna clear the set each time clearly I should just make a new one. No reason to keep it lingering around in memory.
@distractedmosfet
Copy link
Contributor Author

Also I apologize for not including the emoji in the first commit, my brain just skipped over it like it wasn't there. You lot have a system going on! I'm coming in here and messing it up.

@codecov
Copy link

codecov bot commented Sep 15, 2022

Codecov Report

Base: 93.19% // Head: 93.19% // No change to project coverage 👍

Coverage data is based on head (45bbb81) compared to base (855067e).
Patch coverage: 100.00% of modified lines in pull request are covered.

Additional details and impacted files
@@           Coverage Diff           @@
##           master      #43   +/-   ##
=======================================
  Coverage   93.19%   93.19%           
=======================================
  Files          25       25           
  Lines        1351     1351           
=======================================
  Hits         1259     1259           
  Misses         92       92           
Flag Coverage Δ
pytests 93.19% <100.00%> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
mdit_py_plugins/anchors/index.py 100.00% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@chrisjsewell
Copy link
Member

Thanks, sorry for the delayed response!
Yeh I can't think why I put in there in the first place 🤔 but the fix seems to track with https://github.com/valeriangalliat/markdown-it-anchor/blob/8918e4eba26df9b13819acde5a9bf4e4fb8d9816/index.js#L37 👍

@chrisjsewell chrisjsewell changed the title FIX: Anchor ids in separate renders should not affect each other. 🐛: Anchor ids in separate renders should not affect each other. Sep 15, 2022
@chrisjsewell chrisjsewell merged commit e96d4e4 into executablebooks:master Sep 15, 2022
@welcome
Copy link

welcome bot commented Sep 15, 2022

Congrats on your first merged pull request in this project! 🎉
congrats

Thank you for contributing, we are very proud of you! ❤️

@distractedmosfet
Copy link
Contributor Author

Thanks, sorry for the delayed response! Yeh I can't think why I put in there in the first place thinking but the fix seems to track with https://github.com/valeriangalliat/markdown-it-anchor/blob/8918e4eba26df9b13819acde5a9bf4e4fb8d9816/index.js#L37 +1

Sweet! No worries about the delay, just happy to have contributed back :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants