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

changed dynamic imports using exec to static imports #1046

Closed
wants to merge 1 commit into from

Conversation

clinssen
Copy link
Contributor

This addresses #564

@stinebuu stinebuu added T: Enhancement New functionality, model or documentation ZC: Infrastructure DO NOT USE THIS LABEL ZP: PR Created DO NOT USE THIS LABEL S: Normal Handle this with default priority I: No breaking change Previously written code will work as before, no one should note anything changing (aside the fix) labels Oct 25, 2018
@terhorstd terhorstd requested a review from babsey October 29, 2018 11:32
@terhorstd
Copy link
Contributor

this one is related to #1058 and should be checked for compatibility

Copy link
Contributor

@babsey babsey left a comment

Choose a reason for hiding this comment

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

It looks fine to me and I do not know the reason why to import submodules dynamically (@jougs 8d8b19c).

However I approve it.

@jougs jougs self-requested a review November 14, 2018 12:36
@jougs
Copy link
Contributor

jougs commented Nov 14, 2018

Is Topology still working with this fix in place? The reason for the dynamic import was to support extension modules that install an optional Python component. I don't see how they'd be working with a purely static import.

@clinssen
Copy link
Contributor Author

Abandoning this PR due to on-point issues raised by @jougs!

We could potentially switch to a static import of the existing api files and a dynamic import in a special directory containing only user modules, but this is probably better handled later/as part of a different PR, because of active refactoring in PyNEST.

@clinssen clinssen closed this Dec 10, 2018
@babsey
Copy link
Contributor

babsey commented Dec 11, 2018

static import of the existing api files and a dynamic import in a special directory containing only user modules

I like the idea of having static and dynamic imports.

@apeyser
Copy link
Contributor

apeyser commented Mar 14, 2019

It shouldn't be exec, but it shouldn't be static either, but should use importlib module (which probably doesn't fix PyCharm, but it's still cleaner than this).

Should be a different PR on this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
I: No breaking change Previously written code will work as before, no one should note anything changing (aside the fix) S: Normal Handle this with default priority T: Enhancement New functionality, model or documentation ZC: Infrastructure DO NOT USE THIS LABEL ZP: PR Created DO NOT USE THIS LABEL
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants