Skip to content

Conversation

rpkilby
Copy link
Contributor

@rpkilby rpkilby commented Oct 25, 2018

Hi all. I'd like to suggest a few changes, each detailed in individual commits below. Most of them are pretty minor, but I kept them separate for readability's sake.

The only changes that are likely controversial are:

  • Removing requirements.txt in favor of inlining the contents of install_requires. Given that DRF is the only requirement, the file inclusion/parsing seems like overkill.

  • Removing the DRF version bounds. From the commit message:

    New versions of DRF are more likely than not to be compatible. Instead
    of pessimistically restricting compatibility with future DRF versions,
    compatibility should be provided through documentation and explicit
    version bounds in test.

    It might be worth adding a note along the lines of "djangorestframework-gis is generally compatible with modern versions of Django REST Framework, however it's only explicitly tested against the latest version of DRF at the time of release, indicated in the compatibility matrix below."

    Edit: it looks like there was a compatibility issue when upgrading DRF 3.9. I don't fully understand the issue, but my understanding is that it was related to test output only, and doesn't reflect a change in compatibility or expected output from djangorestframework-gis.

This is an unnecessary level of complexity for a requirements list
containing one to two items.
New versions of DRF are more likely than not to be compatible. Instead
of pessimistically restricting compatibility with future DRF versions,
compatibility should be provided through documentation and explicit
version bounds in test.
This is more compact and readable
@rpkilby
Copy link
Contributor Author

rpkilby commented Nov 3, 2018

Rebased off of #173, which fixes the test issue w/ DRF 3.9.x

@auvipy auvipy merged commit 924ba3a into openwisp:master Nov 3, 2018
@rpkilby rpkilby deleted the suggestions branch November 3, 2018 04:37
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