Fixed MyPy errors in upgrade-to-1 PR#1
Closed
Yoshanuikabundi wants to merge 1 commit intoaleivag:upgrade-to-1from
Closed
Fixed MyPy errors in upgrade-to-1 PR#1Yoshanuikabundi wants to merge 1 commit intoaleivag:upgrade-to-1from
Yoshanuikabundi wants to merge 1 commit intoaleivag:upgrade-to-1from
Conversation
bceccca to
b182b7f
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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_warningsproblem, and eventually settled on this one. It just adds a check to your implementation ofcreate_warningsso that it can accept warning enums from either package, and then changes calls toself.create_warnings(...)inrender.pytocreate_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_warningmethod to theMditRenderMixinclass, but that doesn't work (as you probably know) because it is overwritten by child classes. I also tried to just makeMySTNBWarningsa subclass ofMySTWarnings, 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.pyto 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 toconfig.pyto 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!