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

Fixed MyPy errors in upgrade-to-1 PR #1

Closed

Conversation

Yoshanuikabundi
Copy link

Hi @aleivag! Thanks for the work you're doing on upgrading MyST-NB to MyST Parser 1.0.0 - I am sneakily using your branch in some of my docs and it seems to work! I noticed that your PR had stalled, and I have some knowledge about Python's typing system so I thought I'd chip in. Hence this PR to your PR!

I hope you don't feel like I'm stepping on your toes - I haven't contributed to MyST before, so I don't really have any special knowledge about what the maintainers will like. So I thought I'd send this to you rather than directly to MyST NB - if you think this is no good, no worries :)

I experimented with a few solutions to the create_warnings problem, and eventually settled on this one. It just adds a check to your implementation of create_warnings so that it can accept warning enums from either package, and then changes calls to self.create_warnings(...) in render.py to create_warnings(self.document, ...). This way the underlying MyST Parser code doesn't ever see the MyST NB warnings, and the MyST NB code can accept either type. It's also a very minimal change that will continue to work as expected into the future, and it reduces the weirdness of relying on a child class to implement a method you need without declaring it abstract... but now we're getting into advantages that probably only I care about.

I experimented with adding a similarly polymorphic create_warning method to the MditRenderMixin class, but that doesn't work (as you probably know) because it is overwritten by child classes. I also tried to just make MySTNBWarnings a subclass of MySTWarnings, but TIL that python Enums can't do that (which I guess makes sense). I think the solution in this PR is as good as it's going to get - it's frustrating that MyST Parser didn't think about how downstream packages would want to extend the warning system!

I also updated myst_nb/core/config.py to work with the new warnings system, which also included adding a few new variants to your warnings enum. Tests pass locally (except for docutils.py, which I couldn't get to run), and all the pre-commit stuff is passing!

A lot of the line changes are from adding the __future__ import to config.py to match the other modules... PyUpgrade did a lot of stuff automatically. I'm happy to revert that change if it's too much noise!

Hope this is useful! If you'd prefer I open a PR with MyST-NB directly, I'm happy to do that too - just wanted to make sure you have control over the work you've already done. And I'm happy to write some tests to get the coverage up if that ends up being a blocker, or to try and figure out why RTD is complaining.

Thanks!

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.

1 participant