executablebooks / mdformat-myst

Mdformat plugin for MyST compatibility
MIT License
7 stars 2 forks source link

πŸ› FIX: footnote indentations #8

Closed chrisjsewell closed 3 years ago

codecov-commenter commented 3 years ago

Codecov Report

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

welcome[bot] commented 3 years ago

Congrats on your first merged pull request in this project! :tada: congrats
Thank you for contributing, we are very proud of you! :heart:

hukkin commented 3 years ago

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

hukkin commented 3 years ago

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

chrisjsewell commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

because I wasn't fully happy with the solution

maybe have it as a context manager:

with context.indented(4):

hukkin commented 3 years ago

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" :smile:

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.