Refactor to avoid cycle #423
Merged
Conversation
This file contains 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
|
Note — the most important change for demonstrating the improvement is in |
|
LGTM, but need rebase. |
Carreau
approved these changes
Sep 21, 2016
| from .base import (export, get_exporter, | ||
| get_export_names , ExporterNameError) | ||
|
|
||
| from .exporter import Exporter |
There was a problem hiding this comment.
Nitpicking: Exporter is unused, but I guess that would brea API to remove it ?
There was a problem hiding this comment.
Exactly. So should we keep it?
There was a problem hiding this comment.
Sure. Let's merge once you get your rebase right.
mpacer
force-pushed
the
refactor_to_avoid_cycle
branch
3 times, most recently
from
fb52cf4
to
5eaddad
Compare
|
BTW mike can you try to install this |
|
done! |
|
Good.
That was to show that with the extensions you can see the files coverage: And per line coverage in the diff:
|
vidartf
added a commit
to vidartf/jupyterlab_nbconvert_nocode
that referenced
this pull request
Apr 23, 2021
[This change in nbconvert](jupyter/nbconvert#1273) meant that importing `from nbconvert.nbconvertapp import NbConvertApp` in the global scope causes a circular import. That change seems to undo the efforts of jupyter/nbconvert#423, so there might be case to be made to clean it up on the nbconvert side, but this change should at least make this module workable with nbconvert 6 for now.
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.
So this addresses the same issues as #417 but tries to do so by abstracting the hardcoded, deprecated exporter locating into a separate module from the entrypoint based exporter functionality which is not deprecated and instead is maintained in
base.py.The only thing I don't like is that it introduces a new file that is strictly speaking deprecated because thought it is only acting as a shim for the export module that doesn't conflict with the name-space, all of its contents are deprecated.
Another solution would be to just leave all the deprecated functionality inside
export.pysince it's deprecated anyway. I just worry that people might still use export and as a result be confused (and the ambiguity will strictly speaking continue to be present either way). I can make that change but it's not what I'd originally proposed, so I'll make the PR originally of what I proposed, and then will change it if that accords with others' intuitions about which is better.