Django 1.7 and Python3 compatibility

Luthaf commented 10 years ago

This fix compatibility mode for Django1.7 and Python 3.

Luthaf commented 10 years ago

Tests mainly pass with Python 3.4 and 2.7 (Django 1.7 every time). The only failure is in this function. The traceback is

FAIL: test__unicode__ (background_task.tests.TestBackgroundDecorator)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/Volumes/Aldith/Developpeur/django-background-task/background_task/tests.py", line 72, in test__unicode__
    unicode(proxy))
AssertionError: 'TaskProxy(background_task.tests.empty_task)' != '<background_task.tasks.TaskProxy object at 0x1086423c8>'
- TaskProxy(background_task.tests.empty_task)
+ <background_task.tasks.TaskProxy object at 0x1086423c8>

Is this test really important ?

Luthaf commented 9 years ago

Bump. If you don't want to have this, I can use my own fork.

SonOfLilit commented 9 years ago

@Luthaf thanks!

@lilspikey are there plans to merge this?

Luthaf commented 9 years ago

Bump. Can you please merge it, or state clearly that you do not whant it ? I just ran into this bug while trying to deploy a server.

lilspikey commented 9 years ago

Sorry, been bad timing with this. I became a Dad again on the 1st of September and work has been hectic too. As I've not personally been using Django 1.7 or Python 3 this hasn't been a priority for me...

I would like to see this merged, but I've not got a lot of time right now.

RE: the failing test. I think this is down to difference between str and unicode in Python 3?

Re: removing autocommit - which version of Django deprecated that? I've got large sites that are on older versions of Django using this, so don't want to break things if I can avoid it. Might need to add in some sort of compatibility layer for that.

Hopefully we can work together to get this sorted.

cheers,

John

On 10 March 2015 at 21:35, Luthaf notifications@github.com wrote:

Bump. Can you please merge it, or state clearly that you do not whant it ? I just ran into this bug while tying to deploy a server.

— Reply to this email directly or view it on GitHub https://github.com/lilspikey/django-background-task/pull/12#issuecomment-78153496 .

http://psychicorigami.com/

Luthaf commented 9 years ago

Congratulations !

I'll have a look about the failing test.

autocommit was deprecated in django 1.6 I think, and is now the default. See https://docs.djangoproject.com/en/1.7/topics/db/transactions/. I can add a small wrapper around this to check for Django version.

Luthaf commented 9 years ago

Ok, test is fixed, and wrapper for Django compatibility is made !

Your turn now =)

lilspikey commented 9 years ago

Ok, just started looking at this.

Failing on the first hurdle with tests though.

I'm running this against Django 1.4.3 (just happens to be a version I have in a virtual env).

Tests script changes assume that both python2 and python3 installed - I used virtualenv normally, so effectively only one version of python at a time. I'd suggest just use "python" instead of "python2" and "python3" and then use virtualenv to be able to test both.
The autocommit compatibility check fails the moment you have a version number that cannot be parsed as a float (e.g. 1.4.3). Also don't forget that version numbers may contain letters... You should instead check to see if the autocommit function exists or not (see code below):

if hasattr(transaction, 'autocommit'):
    compatibility_autocommit = transaction.autocommit
else:
    # Autocommit is deprecated and the default
    def compatibility_autocommit(function):
        def wrapper(*args, **kwargs):
            return function(*args, **kwargs)
        return wrapper

You've changed get_query_set to get_queryset, which doesn't exist prior to Django 1.6 - so I've got failing tests. We need a compatibility layer for that - maybe just use all() instead of get_queryset?

Might be some other issues, but that's what I've got so far. Figured I had a spare half out, so I'd have a quick look.

cheers,

John

Luthaf commented 9 years ago

I'm running this against Django 1.4.3

Are you sure you still need compatibility for this version of Django ? The 1.6 version has been deprecated by the Django team recently.

You should instead check to see if the autocommit function exists or not

It will exist in all the cases, as it is only deprecated for now.

All the other comments are now fixed !

lilspikey commented 9 years ago

Django 1.4 is a long term release (support for three years), so it's still supported. So I would like to support it in this - particularly given I have production code running with Django 1.4.

On 30 April 2015 at 22:40, Luthaf notifications@github.com wrote:

I'm running this against Django 1.4.3

Are you sure you still need compatibility for this version of Django ? The 1.6 version has been deprecated by the Django team recently.

You should instead check to see if the autocommit function exists or not

It will exist in all the cases, as it is only deprecated for now.

All the other comments are now fixed !

— Reply to this email directly or view it on GitHub https://github.com/lilspikey/django-background-task/pull/12#issuecomment-97976327 .

http://psychicorigami.com/

Luthaf commented 9 years ago

Ok, I didn't knew that. Tell me if you have more comments on this PR !

lilspikey commented 9 years ago

Ok, so that seems to all pass tests ok for Django 1.4 now.

I'd suggest changing the unlock method (to make it a bit more legible from):

    def unlocked(self, now):
        max_run_time = getattr(settings, 'MAX_RUN_TIME', 3600)
        if LooseVersion(django.get_version()) > LooseVersion("1.6"):
            qs = self.get_queryset()
        else:
            qs = self.get_query_set()
        expires_at = now - timedelta(seconds=max_run_time)
        unlocked = Q(locked_by=None) | Q(locked_at__lt=expires_at)
        return qs.filter(unlocked)

To this:

    def unlocked(self, now):
        max_run_time = getattr(settings, 'MAX_RUN_TIME', 3600)
        qs = self.all()
        expires_at = now - timedelta(seconds=max_run_time)
        unlocked = Q(locked_by=None) | Q(locked_at__lt=expires_at)
        return qs.filter(unlocked)

Basically replace the call to get_queryset() with all(), so you can avoid having a version check in the middle there (which makes things look messy).

simonv3 commented 8 years ago

Hey @lilspikey & @Luthaf, is that last change all that's needed to get this merged in? If neither of you have time I can quickly make that change.

Luthaf commented 8 years ago

If you want to do it, I'll merge any PR to my fork (which will update this one). I effectively don't have time for this.

simonv3 commented 8 years ago

I've since found this fork that works on Django 1.7 & Python 3: https://github.com/arteria/django-background-tasks

Luthaf commented 8 years ago

Closing this as their is no chance to get it merged, and I am not using this code anymore. For anyone interested in it, please see the fork linked by @simonv3.

lilspikey / django-background-task

Django 1.7 and Python3 compatibility #12