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.

Sign up for GitHub

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

Jump to bottom

Support Django2.2/3.2, Python3.7/8/9 #496

Merged
merged 3 commits into from
Feb 11, 2022
Merged

Support Django2.2/3.2, Python3.7/8/9 #496

merged 3 commits into from
Feb 11, 2022

Conversation

Jdsleppy
Copy link
Contributor

@Jdsleppy Jdsleppy commented Feb 3, 2022 •
edited

  • Stop use of deprecated BaseMessage.connection
  • celery.task has been removed, use shared_task
  • Remove references to Python 2 compatibility utils
  • Update django-tables2 to a post-Python-2 version
  • Remove usages of deprecated Django features
  • Specify default_auto_field to avoid unintended migrations
  • Use celery>=5.2.1 in tests to avoid deprecation warnings
  • Drop support for EOL Python 3.6
  • tox passes all checks

@Jdsleppy
Support Python 3.7/3.8, Django 2.2/3.2
97fec92 
- Stop use of deprecated BaseMessage.connection
- celery.task has been removed, use shared_task
- Remove references to Python 2 compatibility utils
- Update django-tables2 to a post-Python-2 version
- Remove usages of deprecated Django features
- Specify default_auto_field to avoid unintended migrations
- Use celery>=5.2.1 in tests to avoid deprecation warnings
- Drop support for EOL Python 3.6
@Jdsleppy Jdsleppy force-pushed the feature/support-django32-python38 branch from 7161e7f to 97fec92 Compare February 3, 2022 17:39
Jdsleppy
Jdsleppy commented Feb 3, 2022
View reviewed changes
rapidsms/apps/__init__.py
class RapidsmsConfig(AppConfig):

name = "rapidsms"
default_auto_field = "django.db.models.AutoField"
Copy link
Contributor Author

@Jdsleppy Jdsleppy Feb 3, 2022 •
edited

Choose a reason for hiding this comment

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

Avoids unintended migrations when the default value is eventually changed to BigAutoField.

We may eventually want to have this migration intentionally, though, for applications that may send >2.1 billion messages.

Jdsleppy
Jdsleppy commented Feb 3, 2022
View reviewed changes
rapidsms/backends/http/tests.py
@@ -116,14 +115,14 @@ def test_valid_post_message(self):
message = self.inbound[0]
self.assertEqual(data['text'], message.text)
self.assertEqual(data['identity'],
message.connection.identity)
message.connections[0].identity)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

rapidsms itself has already deprecated message.connection in favor of message.connections[0].

Jdsleppy
Jdsleppy commented Feb 3, 2022
View reviewed changes
rapidsms/contrib/registration/views.py
@@ -85,12 +84,8 @@ def contact_bulk_add(request):
bulk_form = BulkRegistrationForm(request.POST)

if request.method == "POST" and "bulk" in request.FILES:
# Python3's CSV module takes strings while Python2's takes bytes
if six.PY3:
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This will always be True now, so keep this branch.

Jdsleppy
Jdsleppy commented Feb 3, 2022
View reviewed changes
rapidsms/router/api.py
@@ -60,7 +59,7 @@ def send(text, connections, **kwargs):
in :setting:`RAPIDSMS_ROUTER`.
:rtype: :py:class:`~rapidsms.messages.outgoing.OutgoingMessage`
"""
if not isinstance(connections, collections.Iterable):
if not isinstance(connections, collections.abc.Iterable):
Copy link
Contributor Author

Choose a reason for hiding this comment

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

collections.Iterable is deprecated.

Jdsleppy
Jdsleppy commented Feb 3, 2022
View reviewed changes
rapidsms/router/celery/tasks.py
from celery.utils.log import get_task_logger
from rapidsms.errors import MessageSendingError


logger = get_task_logger(__name__)


@celery.task
@shared_task
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Celery does not provide celery.task in modern versions.

Jdsleppy
Jdsleppy commented Feb 3, 2022
View reviewed changes
rapidsms/tests/harness/base.py
@@ -13,7 +11,7 @@
__all__ = ('CreateDataMixin', 'LoginMixin')


UNICODE_CHARS = [unichr(x) for x in xrange(1, 0xD7FF)]
UNICODE_CHARS = [chr(x) for x in range(1, 0xD7FF)]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

https://six.readthedocs.io/#six.unichr

"This is equivalent to unichr() on Python 2 and chr() on Python 3."

Jdsleppy
Jdsleppy commented Feb 3, 2022
View reviewed changes
rapidsms/tests/harness/base.py
@@ -38,7 +36,7 @@ def random_unicode_string(self, max_length=255):
:param length: Length of generated string.
"""
output = ''
for x in xrange(random.randint(1, max_length / 2)):
for x in range(random.randint(1, max_length / 2)):
Copy link
Contributor Author

Choose a reason for hiding this comment

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

https://six.readthedocs.io/#module-six.moves

Jdsleppy
Jdsleppy commented Feb 3, 2022
View reviewed changes
setup.py
@@ -17,10 +17,9 @@ def read_file(filename):

install_requires=[
"requests>=1.2.0",
"django-tables2==1.21.*",
"django-tables2>=2.1.1",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Older versions of django-tables2 relied on Python 2 compatibility utilities within Django that have been removed from Django 3.2.

Jdsleppy
Jdsleppy commented Feb 3, 2022
View reviewed changes
tests/default.py
@@ -118,3 +118,5 @@

CELERY_ALWAYS_EAGER = True
CELERY_EAGER_PROPAGATES_EXCEPTIONS = True

DEFAULT_AUTO_FIELD = 'django.db.models.AutoField'
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This solves some warnings during test, but has no impact on library users.

@Jdsleppy Jdsleppy marked this pull request as ready for review February 3, 2022 17:47
@Jdsleppy Jdsleppy changed the title Support Django2.2/3.2, Python3.7/3.8 Support Django2.2/3.2, Python3.7/8/9 Feb 11, 2022
@Jdsleppy Jdsleppy force-pushed the feature/support-django32-python38 branch from 507b4b6 to c79a645 Compare February 11, 2022 19:44
tobiasmcnulty
tobiasmcnulty approved these changes Feb 11, 2022
View reviewed changes
Copy link
Member

@tobiasmcnulty tobiasmcnulty left a comment

Choose a reason for hiding this comment

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

Looks good and works for me. 🥇

  py37-dj22: commands succeeded
  py37-dj32: commands succeeded
  py38-dj22: commands succeeded
  py38-dj32: commands succeeded
  py39-dj22: commands succeeded
  py39-dj32: commands succeeded
  py39-migrations: commands succeeded
  py39-docs: commands succeeded
  py39-flake8: commands succeeded
  py39-coverage: commands succeeded
  congratulations :)

@tobiasmcnulty tobiasmcnulty merged commit a77e96e into rapidsms:develop Feb 11, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tobiasmcnulty tobiasmcnulty tobiasmcnulty approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@Jdsleppy @tobiasmcnulty