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

Django 1.7 compatibility #112

Closed
wants to merge 18 commits into from
Closed

Django 1.7 compatibility #112

wants to merge 18 commits into from

Conversation

thoas
Copy link
Contributor

@thoas thoas commented Apr 14, 2014

  • Export signals handling to models.py
  • Export all settings to one unique file
  • Export keyfmt to utils.py
  • We are not importing models anymore in init.py which can cause in several cases multiple fails from django.apps Registry
  • Refactor unittests using django own DiscoverRunner

Currently all tests pass except SwitchTests.test_no_query which I'm missing something why we should make on query as we don't cache query from Django itself (we only cache proxies from waffle.__init__.py).

I'm available if you want more information.

This PR is currently under development and not available to merge btw.

Export signals handling to models.py
Export all settings to one unique file
Export keyfmt to utils.py
We are not importing models anymore in __init__.py which can
cause in several cases multiple fails from django.apps Registry
@@ -7,7 +8,10 @@
DEBUG = True
TEMPLATE_DEBUG = True

TEST_RUNNER = 'django_nose.runner.NoseTestSuiteRunner'
if django.VERSION <= (1, 6):

Choose a reason for hiding this comment

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

@thoas – I think this should be <? django.test.runner.DiscoverRunner was added in 1.6.

@ashchristopher
Copy link

@thoas, @jsocol Is this actively being worked on? In addition to the above (I take @thoas's word for it), the migrations need to be converted to support both Django 1.7 (using Django migrations) and <= Django 1.6 using south_migrations.

Happy to create a patch - but this PR looks out of date.

@thoas
Copy link
Contributor Author

thoas commented Oct 16, 2014

@ashchristopher this PR is up to date and synced with the master of @jsocol

@ashchristopher
Copy link

@thoas yes - but you are missing move of the migrations folder to south_migrations and then generation of new migrations.

@thoas
Copy link
Contributor Author

thoas commented Oct 17, 2014

@ashchristopher you are right, will do it.

@thoas
Copy link
Contributor Author

thoas commented Oct 17, 2014

Migrations have been moved to south_migrations with a mention in docs.

I'm still getting test errors from travis, when I execute tox in local all environments passed.

screen shot 2014-10-17 at 10 38 31 am

@thoas
Copy link
Contributor Author

thoas commented Oct 17, 2014

@jsocol could we have your input on this PR plz?

@jsocol
Copy link
Collaborator

jsocol commented Oct 19, 2014

Hey all, sorry I haven't had a chance to dig into this. I'm going to carve out a day next weekend for catching up on PRs. Then I can dive in and look at getting this and a few other things merged.

necessary tables, you'll need to customize the ``SOUTH_MIGRATION_MODULES``
setting: ::

SOUTH_MIGRATION_MODULES = {
Copy link
Collaborator

Choose a reason for hiding this comment

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

We need to add this setting to test_settings, too.

@jsocol
Copy link
Collaborator

jsocol commented Oct 26, 2014

There is a lot going on here. I'm not sure which parts are necessary for 1.7:

  • I'm guessing dropping django-nose/nose was for django-nose's lack of 1.7 support.
  • Were the settings and keyfmt moves necessary?
  • Were the signal handler moves necessary?
  • There are a bunch of outstanding pep8 issues (line length mostly) introduced here. (I'm ignoring the management commands and migrations.)
  • We need 1.7-compatibile initial migrations. It'd be best to add it to to run.sh schema

@jsocol jsocol added this to the 0.10.1 milestone Oct 27, 2014
@thoas
Copy link
Contributor Author

thoas commented Oct 29, 2014

  • Yes, the drop of django-nose/nose was due to a lack of 1.7 support and this runner was definitely not needed
  • I have moved keyfmt to its own file because it's an utility method, I can revert if you think it's not necessary
  • The move of signals was necessary, importing models logic in __init__.py can fails when using custom user (apps not ready, etc.)
  • I have fixed the pep8 issues
  • I have added makemigrations task to run.sh

@thoas
Copy link
Contributor Author

thoas commented Oct 29, 2014

@jsocol I have synced my branch with the current master.

@jsocol
Copy link
Collaborator

jsocol commented Oct 30, 2014

Do you have any idea what's up with the test failures? They seem like actual failures, not errors, but they are crazy sporadic over the past few builds.

@thoas
Copy link
Contributor Author

thoas commented Oct 30, 2014

All tests passed on tox in all environments in local, I do not have any idea of what's going wrong with travis.

I have spend a lot of time but haven't found anything. You are right on the fact there are actually failures.

@thoas
Copy link
Contributor Author

thoas commented Nov 5, 2014

Any follow-up on this PR?

@jsocol could you tell me if you are planning to merge it or not (no big deal if not ;))?

Thank you.

@jsocol
Copy link
Collaborator

jsocol commented Nov 5, 2014

I can't merge it with the inconsistent test failures.

This got pretty big and gnarly. I'd rather take the work done here and spread it out over smaller commits/pull reqs. E.g. drop django-nose without touching anything else, move the signal handlers, do the setting refactor, etc, all separately, before making the actual 1.7 changes last.

Hopefully we just won't run into the test issues, but if we do, it should be a lot easier to suss out what's wrong in smaller branches.

I actually have a little time this week so I'm happy to do that, taking this pull req as a guide. So I'm going to close out this pull req and I might cherry-pick commits out of it, but even if I can't, thank you for doing all the leg work of figuring out what related changes need to happen!

@jsocol jsocol closed this Nov 5, 2014
@thoas
Copy link
Contributor Author

thoas commented Nov 5, 2014

Ok I understand, no problem.

Let me get some time this week to dispatch this PR into multiple PRs as you said.

I will ping you.

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.

None yet

4 participants