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

get_class fails with top level packages. #614

Closed
wants to merge 1 commit into from

Conversation

maiksprenger
Copy link
Member

Hello.
In an attemp to extend the shipping "app" we came into a problem.

We copied the shipping package into our project's package. So we have
MyProject/
shipping/
repository.py
MyProject/
settings.py
....

But after several tries and debugging, the get_class (and more precisely, get_classes) in oscar/core/loading.py was not picking the classes in this package and son, it defaulted to the oscar's system wide shipping module.

After a grep in oscar, it seems that everytime that Repository is required, it is loaded with get_classs. So it should not need any additional bonding other than overriding in INSTALLED_APPS:

INSTALLED_APPS = INSTALLED_APPS + get_core_apps(['shipping'])

It turns out that when get_classes generating the module path, if it is a top level package, it will generate something like : shipping.shipping.repository instead of shipping.repository.

The problem is that _get_app_module_path assumes there's always a dot in the module's name. get_classes should check that there is a dot in the module label before calling _get_app_module_path.

@codeinthehole
Copy link
Contributor

Yes, this is a known issue. Oscar assumes you are using apps packages as somepackage.myapp.

I'll make sure this is documented properly.

Thanks for reporting.

maiksprenger added a commit that referenced this pull request May 2, 2013
@maiksprenger
Copy link
Member

Nobody likes writing documentation, and it's easy enough to fix ;)

@codeinthehole
Copy link
Contributor

@maikhoepfel Could you write some tests to verify this behaviour?

@ghost ghost assigned maiksprenger May 9, 2013
@maiksprenger
Copy link
Member

Okay, added some tests.

@ghost ghost assigned codeinthehole May 17, 2013
@codeinthehole codeinthehole deleted the issue/614/getclass branch January 13, 2014 17:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants