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

Resolve name conflict between function and module. #417

Closed
wants to merge 1 commit into from

Conversation

Carreau
Copy link
Member

@Carreau Carreau commented Sep 17, 2016

nbconvert.exporters.export could be resolved either as a function or a
midule. ExporterNameError would also trigger some circular imports, so
moved it one level higher.

Should Fix #415

CC @michaelpacer

We should audit our imports.

@mpacer
Copy link
Member

mpacer commented Sep 19, 2016

This doesn't solve the circular import issue because you still need to import from each of the exporter classes. I don't know if this is really a good long term solution to this problem which seems more architectural in nature.

The issue is that right now getting an exporter requires telling it where all the exporters are located, but sometimes the function of getting the exporter requires relying on the same code that tells it where it's located. I vote against merging this though it solves he ambiguity issue. It does so by further obscuring this underlying problem of a circular logical dependency not just a circular namespace dependency.

That said I'm having difficulty seeing thinking through how to redesign this at all let alone without breaking API.

That said if it requires breaking API it's a good thing that a major version is about to come out. :)

@Carreau
Copy link
Member Author

Carreau commented Sep 19, 2016

AFAIU the circular import is also due to us exporting function export_<name> right ? IIRC these should be deprecated as well and in the end we should mostly rely on entry-points, and using importers directly.
So once we remove the convenience function, the cyclic import should be gone ?

One solution is to release 5.0 soon, even with this weird API, and to make a 6.0 which is Python 3 only later ? (though, notebook server rely on nbconvert so unsure about that)

nbconvert.exporters.export could be resolved either as a function or a
midule. `ExporterNameError` would also trigger some circular imports, so
moved it one level higher.

Should Fix jupyter#415

# Conflicts:
#	nbconvert/exporters/export.py
@mpacer
Copy link
Member

mpacer commented Sep 20, 2016

AFAIU so long as we have anything where we need to import a function (such as get_exporter) from either .export or .base inside nbconvert/nbconvert/exporters/script.py and we use from .script import ScriptExporter we're going to have a circular dependency issue.

The solution to that (right now) is something like we currently have where we have to put the import inside the function or class that needs it, and that way it will only be imported after the entire module has been loaded.

The question is whether we want to continue doing that or if we want to have export (or whatever replaces it) import get_exporter and ExporterNameError from base.py while still allowing export to use from .script import ScriptExporter but not including that in base.py. Then script.py could also use from .base import get_exporter, ExporterNameError and there would be no cycle because base.py simply wouldn't be pulling in anything from script.py. All of that would happen inside export.py.

Note: what i'm suggesting isn't an alternative to fixing the ambiguous name issue…that's why I included the "or whatever replaces it" because we probably should also rework that. But that requires two new modules rather than one in my solution.

@Carreau
Copy link
Member Author

Carreau commented Sep 20, 2016

let's try to do that, using both base.py and another name. we can even use names that start with underscore to mark them as private and do whatever we need.

@mpacer mpacer mentioned this pull request Sep 20, 2016
them as an nbconvert.exporter entrypoint.
"""
return sorted(entrypoints.get_group_named('nbconvert.exporters'))
<<<<<<< HEAD
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be removed.

@mpacer
Copy link
Member

mpacer commented Sep 20, 2016

I wasn't entirely sure how to incorporate the underscore naming for privateness when most everything is remaining public API. I'm not totally happy with it (as I mention in the notes), but a first pass at what I was thinking is in #423.

@mpacer
Copy link
Member

mpacer commented Sep 22, 2016

should i close this then?

@mpacer mpacer closed this Sep 22, 2016
@mpacer mpacer modified the milestone: 5.0 Oct 20, 2016
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.

Module exports functions with naming conflict with submodule.
2 participants