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

Support django 2.1, DRF 3.9 and use django-filters > 2.0 #173

Merged
merged 7 commits into from
Nov 1, 2018
Merged

Support django 2.1, DRF 3.9 and use django-filters > 2.0 #173

merged 7 commits into from
Nov 1, 2018

Conversation

imomaliev
Copy link
Contributor

@imomaliev imomaliev commented Oct 19, 2018

  • Added python 3.7 to travis and tox
  • removed python2 from travis and tox
  • Had to update postgres for travis to 9.4 and postgis to 2.4 because django 2.1 dropped support for 9.3

also fixes #170

Fails on python 2. Looks like I need to fix #171 as well. Can someone guide me so that I can drop python 2 support. What should be done I see multiple comments like

class GeoJsonDict(OrderedDict):
    """
    Used for serializing GIS values to GeoJSON values
    TODO: remove this when support for python 2.6/2.7 will be dropped
    """

Should GeoJsonDict be removed?

@imomaliev imomaliev changed the title lift drf version to include 3.9 [WIP] lift drf version to include 3.9 Oct 19, 2018
@imomaliev imomaliev changed the title [WIP] lift drf version to include 3.9 Support django 2.1, DRF 3.9 and use django-filters > 2.0 Oct 19, 2018
@auvipy
Copy link
Collaborator

auvipy commented Oct 20, 2018

do you think the pr could split up in several ones to handle the relevant issues in each one?

@imomaliev
Copy link
Contributor Author

imomaliev commented Oct 20, 2018

@auvipy I can't update packages separately, because they are dependent, but I could move addition of python 3.7 to travis and dropping of python 2 support in separate PRs

Copy link
Member

@nemesifier nemesifier left a comment

Choose a reason for hiding this comment

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

@imomaliev welcome and thanks for contributing.

1 question: why are you adding a Dockerfile and a docker-compose.yml?
We don't have enough resources to maintain those as things change, so if anyone tries to use it at some point in the future and it won't work, I personally won't have time to help them and will refer these users to you. Do you have time to maintain that over time? Consider this factor please, I'm not in favour of adding non essentials which need to be maintained by volunteers which are already over burdened.

Copy link
Member

@nemesifier nemesifier left a comment

Choose a reason for hiding this comment

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

@imomaliev welcome and thanks for contributing.

1 question: why are you adding a Dockerfile and a docker-compose.yml?
We don't have enough resources to maintain those as things change, so if anyone tries to use it at some point in the future and it won't work, I personally won't have time to help them and will refer these users to you. Do you have time to maintain that over time? Consider this factor please, I'm not in favour of adding non essentials which need to be maintained by volunteers which are already over burdened.

@imomaliev
Copy link
Contributor Author

imomaliev commented Oct 23, 2018

@nemesisdesign I do have time to maintain it. I would suggest having this in repo, because it simplifies setting up development environment for future contributors. But if you and other maintainers are so strongly against it, I will drop them from this PR

@auvipy So should I split this PR like I suggested in #173 (comment) ?

@imomaliev
Copy link
Contributor Author

@nemesisdesign @auvipy ?

Copy link
Member

@nemesifier nemesifier left a comment

Choose a reason for hiding this comment

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

@imomaliev I'm not against having it, I just know that I won't be able to help if someone in the future reports that the docker image is not working.

Could you rebase your PR against the current master?

@imomaliev
Copy link
Contributor Author

@nemesisdesign rebased

Copy link
Member

@nemesifier nemesifier left a comment

Choose a reason for hiding this comment

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

@imomaliev I was going to merge but after checking the code I found out something which looks like a minor issue, see below

.travis.yml Show resolved Hide resolved
@auvipy auvipy merged commit db7132c into openwisp:master Nov 1, 2018
@nemesifier
Copy link
Member

Thanks @imomaliev @auvipy

@rpkilby rpkilby mentioned this pull request Nov 3, 2018
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.

Drop Python 2.7 from master branch. Upgrade django-filter to 2.0
4 participants