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

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

Open
Saijin-Naib opened this issue Apr 18, 2022 · 17 comments
Open

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

Saijin-Naib opened this issue Apr 18, 2022 · 17 comments

Comments

@Saijin-Naib
Copy link

It looks like there a number of breaking changes in the latest mistune:
lepture/mistune#290

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

@amyreese
Copy link

amyreese commented May 5, 2022

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
Copy link

kloczek commented Jun 22, 2022

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

Why not submit that here as PR? 🤔

@amyreese
Copy link

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

@CrossNox
Copy link
Owner

CrossNox commented Jun 22, 2022

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
Copy link
Author

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
Copy link
Owner

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

@tigerfoot
Copy link

same for packaging it for openSUSE Tumbleweed.

@kloczek
Copy link

kloczek commented Jun 23, 2022

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
Copy link

amyreese commented Jun 23, 2022

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
Copy link

kloczek commented Jun 23, 2022

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
Copy link
Owner

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
Copy link

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
Copy link

kloczek commented Aug 23, 2022

Cannot apply that PR networktocode/diffsync#147 on top last version and master 🤔

@CrossNox
Copy link
Owner

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
Copy link

kloczek commented Aug 24, 2022

So how can I test that PR? 🤔

@matteoferla
Copy link

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
Copy link

Where does this stand? What is the path forward?

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

No branches or pull requests

8 participants