django settings imported incorrectly

[Michael Nelson]

STR:
 * Create a test that overrides/patches various LAZR_RESTFUL_ settings,

Expected result:
 * DjangoWebServiceConfiguration should respect the new settings

Actual result:
 * DjangoWebServiceConfiguration reads the settings directly from the project's settings module.

I'm not certain if it's intentional but in lazr/restful/frameworks/django.py the django settings are imported with:
{{{
import settings
}}}

but this is importing the project's settings.py module directly, rather than getting a handle on the settings via django.conf.settings. I *think* it should be instead doing:

{{{
settings = __import__('django.conf', {}).conf.settings
}}}

like the other django imports in lazr/restful/frameworks/django.py

[Gary Poster (gary)]

Hi Michael. I strongly suspect that you know more Django than any of us; and ISD is the only consumer of the Django code, so if it works for you with these changes, that's what we care about most.

Can you confirm that (a) you think that the __import__ spelling is what we are supposed to be doing and (b) when you make that change, everything else seems to work? If so, I'm happy to approve the change. A branch with the change and tests would make it happen fastest, of course.

Thank you

Gary

[Michael Nelson (michael.nelson)]

Thanks Gary. I've linked a branch, and verified that locally I don't need to manually patch the settings module with this change for testing.

Regarding (a), the comment in the code there says more than I know:

# Without this trickery, this module (called "django") will mask the
# real Django modules. We could use "from __future__ import
# absolute_import", but that would not work with Python 2.4.

So if Python 2.4 is no longer supported (?) we could switch to absolute_import? Or perhaps you know a better way?

Cheers,
Michael

[Michael Nelson (michael.nelson)]

More clearly: "verified that locally I no-longer need to manually patch the settings module for testing when this change is applied to lazr.restful."

[Gary Poster (gary)]

ah! I thought the __import__ thing was some regular practice. I see, it's just about masking.

Yeah, we can dump 2.4; the absolute_import thing should work.

We'll get either your branch or the absolute_import approach into the next release.

[Gary Poster (gary)]

How soon do you need this in a release? hours, days, ...?

[Michael Nelson (michael.nelson)]

There's no rush from ISD's pov (we're not blocked on it).

[Robert Collins (lifeless)]

@michael, can you propose this as a merge proposal (or is it no longer needed?)

[Michael Nelson (michael.nelson)]

@robert, done - but it is not needed for us... as we no longer use lazr.restful on the server.

[Robert Collins (lifeless)]

So, it failed test. As noone needs it, I'm going to just close this ticket.