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

Add support for footnotes #48

Merged
merged 22 commits into from
Aug 11, 2022
Merged

Add support for footnotes #48

merged 22 commits into from
Aug 11, 2022

Conversation

bimbashrestha
Copy link
Member

@bimbashrestha bimbashrestha commented Aug 9, 2022

Closes #37

TODO (will do this once you think this approach looks fine @johndgiese):

  • Periods after the footnote reference are currently gobbled up (easy fix though)
  • Add unit tests for footnotes plugin
  • Add warning for multiple references to the same footnote
  • Add warning if footnote contains no content

@bimbashrestha bimbashrestha changed the title Footnotes Add support for footnotes Aug 9, 2022
Copy link
Contributor

@johndgiese johndgiese left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bimbashrestha why is it necessary to take second pass through the rendering here? It seems like we already kind of have two passes. In the first pass, we're constructing all of the notion objects (e.g., the subclasses of Block, RichTextArray, Mention, etc.). In the second pass we run to_pandoc on everything.

Couldn't we collect all of the footnotes in the constructor phase, say by attaching data to the plugin_data dict on the client (see https://github.com/innolitics/n2y/blob/main/n2y/notion.py#L94) and then using this to look up the foot notes after? Alternatively, it could make name spacing easier of the plugin_data were on the associated Page object.

Also, if we did move forward with a two-pass approach, I think we would want to only do it if there were particular block instances that needed it instead of only relying on block classes that need it. This is because even if you have a plugin with a block that sometimes requires a second pass, I bet most of the time you wouldn't need it. I realize this would be more complicated to do, but again, maybe we can use the constructor phase as the first pass and the to_pandoc as the second pass, and thus avoid this complexity.

I very well may be missing some reason as to why the two pass system you added is necessary though, in which case we should definitely discuss.

@bimbashrestha
Copy link
Member Author

Hey @johndgiese good question. I think I might have misunderstood the data flow. Let me take a second stab at this today.

I suspect you're right and it's possible to do without the second pass logic I have now.

n2y/main.py Outdated Show resolved Hide resolved
Copy link
Contributor

@johndgiese johndgiese left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice work! Please address all the comments and also be sure you can create markdown files for our website for the guidance docs

@bimbashrestha bimbashrestha merged commit c37bdab into main Aug 11, 2022
@johndgiese johndgiese deleted the footnotes branch December 8, 2022 18:01
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.

Add a builtin plugin for supporting footnotes
2 participants