CrossNox / m2r2

Markdown to reStructuredText converter
https://crossnox.github.io/m2r2
MIT License
107 stars 26 forks source link

Inquiry: m2r2 update to support mistune current (v2) #47

Open Saijin-Naib opened 2 years ago

Saijin-Naib commented 2 years ago

It looks like there a number of breaking changes in the latest mistune:
https://github.com/lepture/mistune/issues/290

I'm not sure if tracking the current branch is in scope or not.

amyreese commented 2 years ago

FWIW, I forked sphinx-mdinclude from m2r2, and upgraded the core to support mistune 2.0, and use it with all of my projects now:

https://sphinx-mdinclude.readthedocs.io/en/latest/

However, I did strip out some of the features unrelated to Sphinx support, so it's not necessarily an option for everyone, but thought it might be worth mentioning. If someone wants to port the changes for mistune back to m2r2, it shouldn't be too difficult.

kloczek commented 2 years ago

FWIW, I forked sphinx-mdinclude from m2r2, and upgraded the core to support mistune 2.0

Why not submit that here as PR? πŸ€”

amyreese commented 2 years ago

Why not submit that here as PR? πŸ€”

A few reasons:

CrossNox commented 2 years ago

m2r2 has little dependencies, mistune being the most important. Staying up to date with major versions seems wise, but is a breaking change here. No big deal, though.

I'm inclined to believe that most packages that depend on m2r2 depend on mistune only because the former does.

Does a major release using mistune 2 sound good?

(Edit: not saying I'll cook it right now, rather ASAP as uni is taking most of my free time at least for the next couple weeks).

Saijin-Naib commented 2 years ago

I know for me personally, it'd be helpful. It would allow me to drop a patch for packaging it for Alpine, and hopefully ease integration with Formiko to make an updated build.

CrossNox commented 2 years ago

I'll take a look at @jreese 's fork and see how/what can be used from there

tigerfoot commented 2 years ago

same for packaging it for openSUSE Tumbleweed.

kloczek commented 2 years ago

Why not submit that here as PR? πŸ€”

A few reasons:

  • It's quite an invasive upgrade
  • It was made easier by removing pieces of m2r that I didn't need/use in my own projects
  • I needed a mistune2-compatible release ASAP
  • This project isn't super active, so I wasn't sure a PR would get traction

Do you think that iot is possible to prepare less invasive upgrade for mistune v2?

amyreese commented 2 years ago

Do you think that iot is possible to prepare less invasive upgrade for mistune v2?

mistune 2.0 basically refactors (and renames) the entire API, and splits functionality into new components in multiple places. And of course there are a bunch of subtle differences in behavior for most of these pieces. Upgrading to the new API requires many small changes everywhere. And there's no good way to support both versions without spending even more time/effort to build an otherwise-useless abstraction layer on top.

For reference, compare legacy.py (more or less a cleaned-up version of the mistune portion of m2r2) to the mistune 2.0 equivalents in parse.py and render.py. They mostly share the same vague structure, but a lot of the details are different, and the test suite also had to be audited and updated in the process to account for those subtle behavior changes.

I wouldn't classify any of this as "difficult"β€”just tedious, especially since documentation of mistune is lacking or non-existent. I needed to spend a fair amount of time just reading and debugging both the old and new versions of mistune just to figure out what needed to change and how.

kloczek commented 2 years ago

Because you've mention about "invasiveness" of your adaptation as argument to not submit those changes as PR by asking that question I've been expecting only "yes" or "not". And .. can you prepare PR with such adaptation against this repo?

CrossNox commented 2 years ago

I think the answer provided is way more insightful than a yes/no answer. They have the experience of doing such changes and are better prepared to understand what changes in the parts that they removed from their codebase might be required.

glennmatthews commented 2 years ago

Dependabot is now reporting a need to update to mistune 2.0.3 or later due to a security vulnerability, for what it's worth.

kloczek commented 2 years ago

Cannot apply that PR https://github.com/networktocode/diffsync/pull/147 on top last version and master πŸ€”

CrossNox commented 2 years ago

Yes, this issue is about migrating to mistune v2. So, moving to mistune2.x.x will not be compatible with m2r2 until this is ready and deployed.

Version 0.8.4, upon which m2r2 depends, is affected

kloczek commented 2 years ago

So how can I test that PR? πŸ€”

matteoferla commented 1 year ago

Due to dependency conflicts, I gave the sphinx-mdinclude solution by @amyreese for Sphinx a go and it works nicely (I also changed a SO A which used m2r2 to use that as it's more directly applicable).

ahopkins commented 1 year ago

Where does this stand? What is the path forward?

CrossNox commented 3 weeks ago

WIP