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

Add support for Django 3.1 #65

Merged
merged 5 commits into from
Aug 19, 2020
Merged

Add support for Django 3.1 #65

merged 5 commits into from
Aug 19, 2020

Conversation

WayneLambert
Copy link
Contributor

When trying to install a celery version greater than 4 to a project,
PipEnv cannot resolve the dependency since setup.py is set to <4.0

It is also clear that the package does not currently support Django 3.1

This commit addresses the following issues:

  • Updates the Tox/Travis config to allow for the testing of Django 3.1
    with its supported Python versions (3.6, 3.7, and 3.8)
    See: https://docs.djangoproject.com/en/3.1/faq/install/
  • Updates setup.py to have a constraint for celery of <5.0
  • A couple of refactoring changes:
    • Replace nested if condition with a ternary operator (conditional
      expression)
    • Test params for truthiness instead of its length
  • Ran isort over the project and accepted its sorted imports
  • Updates README.rst to have an example Google Analytics code that is
    aligned with the Google docs and a user's actual tracking code.
  • Added .vscode to the .gitignore file to help VS Code contributors
  • Updated CHANGELOG.rst with the more major points of the change
  • Altered Development Status classifier to 5 - Production/Stable.
    This was agreed in PR Update codebase for Django 3 and Python 3.8 #62 by @rudigiesler

I have NOT changed the version number within setup.py. I will let you
do this upon build/release.

When trying to install a celery version greater than 4 to a project,
PipEnv cannot resolve the dependency since `setup.py` is set to <4.0

It is also clear that the package does not currently support Django 3.1

This commit addresses the following issues:

- Updates the Tox/Travis config to allow for the testing of Django 3.1
  with its supported Python versions (3.6, 3.7, and 3.8)
  See: https://docs.djangoproject.com/en/3.1/faq/install/
- Updates `setup.py` to have a constraint for celery of <5.0
- A couple of refactoring changes:
    - Replace nested if condition with a ternary operator (conditional
      expression)
    - Test params for truthiness instead of its length
- Ran `isort` over the project and accepted its sorted imports
- Updates `README.rst` to have an example Google Analytics code that is
  aligned with the Google docs and a user's actual tracking code.
- Added `.vscode` to the `.gitignore` file to help VS Code contributors
- Updated `CHANGELOG.rst` with the more major points of the change
- Altered `Development Status` classifier to `5 - Production/Stable`.
  This was agreed in PR #62 by @rudigiesler

I have NOT changed the version number within `setup.py`. I will let you
do this upon build/release.
@WayneLambert
Copy link
Contributor Author

Hi,

The build for the PR failed. The following is the pertinent part of the traceback from the Python 3.8 / Django 3.1 config:

  File "/home/travis/build/praekeltfoundation/django-google-analytics/.tox/py38-dj31/lib/python3.8/site-packages/djcelery/admin.py", line 3, in <module>

    from anyjson import loads

ModuleNotFoundError: No module named 'anyjson'

ERROR: InvocationError for command /home/travis/build/praekeltfoundation/django-google-analytics/.tox/py38-dj31/bin/coverage run .tox/py38-dj31/bin/django-admin test (exited with code 1)

___________________________________ summary ____________________________________

ERROR:   py38-dj31: commands failed

Since then, I note on issue #64, @rudigiesler mentioned that version 2.2.5 is the lowest version supported by the package.

Would you like me to submit another PR to supersede this one which updates the travis.yml and tox.ini files to simplify the number of testing configurations before you start to address the actual reason for failure?

Thanks,

Wayne

@rudigiesler
Copy link
Contributor

@WayneLambert Looks good to me. I would add the higher Django version constraint to setup.py though. (currently <3.1)

Happy for us to remove/cleanup testing on any versions that are no longer supported.

Looking at the error, seems like django-celery is to blame: celery/django-celery#568 , it doesn't support celery 4: https://github.com/celery/django-celery/blob/master/setup.py#L184. I'm not sure exactly where it's all being used in this project, but maybe we can remove django-celery and use celery directly, although I do remember changes somewhere with celery that required changes to the configuration, so we might need to set a minimum version for celery.

I'm also happy to have isort added to the CI, to ensure that we don't forget to run it.

Updates oversight from previous commit so that the `install_requires`
config in `setup.py` now has a max version for Django of 3.2

Ref PR: #65
@WayneLambert
Copy link
Contributor Author

Hi @rudigiesler,

I have updated the PR for that change. You're absolutely right - it was an oversight on my part. Of course, the automated build from the PR will obviously still fail.

Yes, I'm more than happy for you guys to update the testing configurations to the supported versions.

Yep, I did think the same with regards to Django Celery. Hopefully it doesn't cause any breakages if you just end up using celery directly.

Good idea about adding isort to the CI.

Thanks,

Wayne

setup.py Outdated Show resolved Hide resolved
Making the package compatible with Celery >4.0 will take more extensive
effort, therefore this has been reversed back to its prior state.

The objective is to make the package compatible with Django >=3.1 and
<3.2 with a view to addressing Celery compatibility at a later stage.

Ref: #65
See also: #64
@rudigiesler
Copy link
Contributor

Cleanup PR has been merged: #66

@rudigiesler rudigiesler changed the title Add support for Django 3.1 and Celery 4.x.x Add support for Django 3.1 Aug 19, 2020
@WayneLambert
Copy link
Contributor Author

OK, that's great. Thanks.

Following the revision for removing the attempt at making it compatible with Celery <5, the checks have now passed for the PR. Is there anything else required or can this be merged and a new release to PyPI?

@rudigiesler
Copy link
Contributor

@WayneLambert Maybe just remove Add support for Celery 4.x.x from the changelog? Other than that, looks good.

Remove entry for `Add support for Celery 4.x.x`

PR: #65
@rudigiesler rudigiesler merged commit 27534c3 into praekeltfoundation:develop Aug 19, 2020
@rudigiesler
Copy link
Contributor

@WayneLambert Release made: https://pypi.org/project/django-google-analytics-app/4.4.2/

WayneLambert added a commit to WayneLambert/portfolio that referenced this pull request Aug 19, 2020
Following my PR being merged in the Django Google Analytics app, a new
release was made to PyPI.

PR:
praekeltfoundation/django-google-analytics#65
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.

2 participants