Skip to content

🐛 FIX: footnote indentations#8

Merged
chrisjsewell merged 3 commits intomasterfrom
fix-footnotes
Jun 23, 2021
Merged

🐛 FIX: footnote indentations#8
chrisjsewell merged 3 commits intomasterfrom
fix-footnotes

Conversation

@chrisjsewell
Copy link
Member

No description provided.

@codecov-commenter
Copy link

Codecov Report

❗ No coverage uploaded for pull request base (master@46d7e08). Click here to learn what that means.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##             master       #8   +/-   ##
=========================================
  Coverage          ?   92.85%           
=========================================
  Files             ?        3           
  Lines             ?      168           
  Branches          ?        0           
=========================================
  Hits              ?      156           
  Misses            ?       12           
  Partials          ?        0           
Flag Coverage Δ
pytests 92.85% <0.00%> (?)

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


Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 46d7e08...ca655df. Read the comment docs.

@chrisjsewell chrisjsewell merged commit bc402d8 into master Jun 23, 2021
@chrisjsewell chrisjsewell deleted the fix-footnotes branch June 23, 2021 13:46
@welcome
Copy link

welcome bot commented Jun 23, 2021

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

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

@hukkin
Copy link
Collaborator

hukkin commented Jun 23, 2021

@chrisjsewell I'd like to review before merging please. Also, I actually prefer the original configuration and Poetry in projects that have dependencies.

@hukkin
Copy link
Collaborator

hukkin commented Jun 23, 2021

Regarding the repo transfer, my thinking was that it happened on similar terms as mdformat.

Copy link
Collaborator

@hukkin hukkin left a comment

Choose a reason for hiding this comment

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

Added a few notes.

Also, there was an undocumented bug where the first line is too wide when --wrap=INTEGER is used. That's why I originally didn't indent the text. This change applies the problem to all lines, which I'm fine with if it fixes a bug, but we should make an issue about that.

body = indent("\n\n".join(elements), " " * 4)
# if the first body element is a paragraph, we can start on the first line,
# otherwise we start on the second line
if body and node.children and node.children[0].type != "paragraph":
Copy link
Collaborator

Choose a reason for hiding this comment

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

the body condition makes me think, what happens if not body. If that is valid syntax, I think it needs separate handling where no trailing space is added?

Copy link
Member Author

Choose a reason for hiding this comment

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

yes it is valid syntax, but yeh I can remove the trailing space. In fact it was removing it, but then I refactored the code slightly and forgot to "rehandle" it

Copy link
Member Author

Choose a reason for hiding this comment

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

actually it only works currently if there is a space, otherwise it is not recognized as a footnote.

I note though, that this is recognized as a footnote in VS Code previews (which uses markdown-it), so perhaps this is a bug in the plugin (albeit a quite unlikely scenario, that you would want a footnote with no text)

@chrisjsewell
Copy link
Member Author

Absolutely happy to wait for your review, but err I would say this is slightly different to mdformat, in that obviously I am moreso the expert on MyST and know what needs to be in this package before I'm happy to start advertising/endorsing it on myst-parser etc.

@chrisjsewell
Copy link
Member Author

Also, there was an undocumented bug where the first line is too wide when --wrap=INTEGER is used

this seems like a bug of mdformat no? It should be able to understand when the paragraph is in an "indented environment".
I note there is also env["indent_width"] in mdformat, but it is not clear to me how/if this should be used by plugins?

@hukkin
Copy link
Collaborator

hukkin commented Jun 23, 2021

this seems like a bug of mdformat no?

No I wouldn't consider this an mdformat bug. I made an issue in mdformat to document env["indent_width"]. Basically that is the way to signal an "indented environment" that you mentioned. Why it's not documented is because I wasn't fully happy with the solution at the time so was hoping nobody would notice it, use it, or mention it, lol, before I come up with a better solution.

@chrisjsewell
Copy link
Member Author

because I wasn't fully happy with the solution

maybe have it as a context manager:

with context.indented(4):

@hukkin
Copy link
Collaborator

hukkin commented Jun 23, 2021

Yeah that needs to be added for sure, thanks for reminding me. I deliberately didn't add the context manager at the time to avoid making the indent system public API and "official" 😄

I think the alternative I considered was a bit reversed logic where there is a mapping from syntax name to indent width of that syntax, and then the paragraph render function, before wrapping, would add up the total indent width based on syntax types of its ancestors.

Now thinking about it, I think the system in place (along with the context manager) is probably the better and more flexible way to do this. So maybe time to just create the context manager and document it.

Anyways, the real problem with footnotes and any of these indent systems is that the first line is special and should be wrapped with a narrower indent width.

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.

3 participants