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

Fix import from ref #149

Merged
merged 1 commit into from
Jul 14, 2016
Merged

Fix import from ref #149

merged 1 commit into from
Jul 14, 2016

Conversation

jarekwg
Copy link
Contributor

@jarekwg jarekwg commented Jul 7, 2016

Hi,

Depending on how a python project is defined it may not always be safe to assume that
from A.B.C import D is equivalent to import A; D = A.B.C.D. Currently the code uses the latter approach, recursively digging down to the function.

The proposed approach behaves more like the python example usage for __import__, which fixes an issue I was having with apscheduler.

Regrettably I'm not sure how to submit a failing test for this, nor proper reasoning for why the current recursive approach can fail in some cases, but hopefully I've provided enough to go on for this to be understood and merged. It's also shorter! Note that I've dropped the .split('.') on rest, though. I'm not sure of this use case, but I'd expect the proposed implementation to cover it too.

Also added isort settings to setup.cfg.

Cheers,
Glow

@agronholm
Copy link
Owner

What problem does this fix?

@agronholm
Copy link
Owner

Also why would I want this isort thing?

@agronholm
Copy link
Owner

The actual change looks like an improvement to how ref_to_obj() handles imports. Please however refrain from adding unrelated changes to the PR. Also, you can add things to sys.modules manually for testing.

@agronholm
Copy link
Owner

Also make sure the test suite passes first.

@jarekwg
Copy link
Contributor Author

jarekwg commented Jul 7, 2016

Apologies, I usually append isort config to my pull requests, and the maintainers happily accept them. But you're right, it's irrelevent to the PR, I'll remove it when I next update.

As for the problem this fixes, I'm setting up APScheduler with a Django project. The path to the function's module is <projectname>/<apps>/<appname>/tasks.py. I define the function there and call scheduler.add_job() to add it. However Job.func_ref never gets set. I've dug through the code and found that it's failing to import tasks from <appname> during the recursive import in ref_to_obj. I'm not sure why this is (perhaps cleverer pythoners than me can explain), however importing the tasks directly in the __import__ call as I've done in this PR fixes it.

Yep, will fix tests. got ahead of myself. Should've run tox locally before pushing.

@coveralls
Copy link

coveralls commented Jul 7, 2016

Coverage Status

Coverage remained the same at 93.061% when pulling e2f780c on jarekwg:master into 9eb5dc5 on agronholm:master.

@jarekwg
Copy link
Contributor Author

jarekwg commented Jul 7, 2016

Looks like the rest part still needed to be recursed.

@agronholm agronholm merged commit 00d01b1 into agronholm:master Jul 14, 2016
@agronholm
Copy link
Owner

Thanks for the patch. I think I can add a test for this on my own.

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.

3 participants