Should we prefer celery tasks rather that threads ?

[dulacp]

I'm referring to the EmailThread class, we probably should change that for django celery tasks if that's available, otherwise use the main thread to perform it, don't you think ?

Moreover, it broke the test suite sometimes for strange reasons...

I'm willing to make to changes in PR if you agree or complete this thought :)

[dominno]

Yes, you are right. I wanted to do this as well. But this should be made as backend class pattern to allow to use order lib then celery if needed in future.

[dominno]

So in settings by default celery backend would be set, if celery is not installed then use main thread backend class.

[dulacp]

The thread backend brings errors to the unit test suite... because a thread work might not be finished when the assert statement is reached. That's why my #93 issue is marked as "failed" but in fact it should have passed, nasty issue with using threads sadly.

I'd thought about celery by default with a fallback to synchronous code on the main thread (available out of the box with celery), then the user might be able to switch the backend class to use a thread one, don't you think it's better ?

[dominno]

Yes, thought exactly the same way.

[dominno]

Just i thought about making some backend class that would use the same interface methods for celery and main thread, so its easy to write other backend class to use different lib then celery if needed in future. Example

class MessageSendBackend(object):

    def send(self):
        raise NotImplementedError

class CeleryMessageSend(MessageSendBackend):

    def send(self):
        # send by celery

Do you understand what i mean ?

[dulacp]

Of course, an adapter pattern (if I'm not mistaken) is the perfect approach, nice idea.

[dominno]

Yes you are right.

[coveralls]

Coverage decreased (-0.19%) when pulling 1f3f1fd4d1b472d3a1cf01b5b84f145e2a06c9cb on dulaccc:message-send-backends into dd6407ddfb060cb27deb25da454145a8d5d25f73 on dominno:master.

[dulacp]

For now I've only added the synchronous email backend, but I will reintegrate the email thread backend for compatibility purposes, and write a celery backend in the future if somebody needs it.

Are you agree that the synchronous email should be the default one ? That imply that if people wants to keep using email threads they will have to specify that by hand, and it maybe needs a explanation in the doc (or the README file)

[dominno]

its ok for synchronous email backend to be default, just please add some info to the readme.

Also please write a test that will test this line https://coveralls.io/files/87149440#L113

[dulacp]

Sorry it's been a busy week for me, I now have some time to finish the work like you've said, README and a missing test.

[dominno]

Please fix PEP8,

$ pep8 --exclude=migrations --ignore=W291 moderation tests tests/tests/unit/moderator.py:59:1: W293 blank line contains whitespace tests/tests/unit/moderator.py:67:80: E501 line too long (86 > 79 characters) tests/tests/unit/moderator.py:72:1: W293 blank line contains whitespace tests/tests/unit/moderator.py:77:80: E501 line too long (81 > 79 characters) tests/tests/unit/moderator.py:78:80: E501 line too long (81 > 79 characters) tests/tests/unit/moderator.py:89:80: E501 line too long (81 > 79 characters) tests/tests/unit/moderator.py:90:80: E501 line too long (81 > 79 characters)

[dulacp]

Oh... I always forget, sorry.

[coveralls]

Coverage increased (+0.09%) when pulling cd81a944818bb9f107df5d12a0b6be9bf81cb54b on dulaccc:message-send-backends into dd6407ddfb060cb27deb25da454145a8d5d25f73 on dominno:master.