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

Paths to subpackages are not resolved by find_module in JyNIImporter #8

Closed
lokuz opened this issue Mar 29, 2016 · 5 comments
Closed

Comments

@lokuz
Copy link

lokuz commented Mar 29, 2016

Subpackagenames are not resolved by find_module in JyNIImporter. The reason is that find_module does not descent into the directories.

This is also relevant for local import paths (from . import multiarray) since they are changed to global paths (numpy.core.multiarray) and handed over to find_module as name.

I implemented a solution for this and will upload it after further testing.

@Stewori
Copy link
Owner

Stewori commented Mar 31, 2016

Great! I'd suggest to file a pull request soon (i.e. a draft); testing does not need to be 100% for now. Then I could already review it and also test. This would also enable deeper discussion of this topic if needed. Since we are still alpha I regard it as 'okay' if it does not break current tests (which should be orthogonal to this feature I suppose). You should add a test that would fail without this patch, but that is not required for the first pull request draft.

@lokuz
Copy link
Author

lokuz commented Mar 31, 2016

Ok, sounds good. Just give me a few days to fork your repo and make a version without all the logging, which I just have in the code to understand, what is going on.

@lokuz
Copy link
Author

lokuz commented Apr 4, 2016

Created a fork and therein the branch feature_import_hook_submoduls with the extended method. I need to add a test and therefore a demo subpackage.

@Stewori
Copy link
Owner

Stewori commented Apr 13, 2016

Can you provide a minimal sample that triggered this issue for you? It's okay for now if the test depends on NumPy. I'd just like to verify your solution under original circumstances, so we can get it merged...

@Stewori
Copy link
Owner

Stewori commented Apr 14, 2016

Never mind...
Should be fixed as of 66a74e0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants