Skip to content

Fixed #27830 -- Used distutils.version.LooseVersion for version parsing. #8068

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
Merged
merged 1 commit into from
Jun 13, 2017
Merged

Fixed #27830 -- Used distutils.version.LooseVersion for version parsing. #8068

merged 1 commit into from
Jun 13, 2017

Conversation

ChillarAnand
Copy link
Contributor

@ChillarAnand ChillarAnand commented Feb 15, 2017 •
edited by timgraham
Loading

@timgraham
Copy link
Member

Why are tests removed? I'd expect more tests to be added to show what cases this fixes.

@ChillarAnand ChillarAnand changed the title Fixed #27830 -- Used distutils.version.LooseVersion for version handling [WIP] Fixed #27830 -- Used distutils.version.LooseVersion for version handling Feb 15, 2017
@ChillarAnand ChillarAnand force-pushed the 27830-version branch 6 times, most recently from 60db412 to 6167dd3 Compare February 15, 2017 17:26
@charettes charettes mentioned this pull request Feb 17, 2017
Closed
@dvarrazzo dvarrazzo mentioned this pull request Feb 17, 2017
Closed
@ChillarAnand ChillarAnand force-pushed the 27830-version branch 7 times, most recently from 94b5c88 to 653200b Compare February 19, 2017 15:43
@ChillarAnand ChillarAnand changed the title [WIP] Fixed #27830 -- Used distutils.version.LooseVersion for version handling Fixed #27830 -- Used distutils.version.LooseVersion for version handling Feb 19, 2017
@ChillarAnand ChillarAnand changed the title Fixed #27830 -- Used distutils.version.LooseVersion for version handling Fixed #27830 -- Used packaging.version.Version for version handling Feb 19, 2017
@ChillarAnand ChillarAnand force-pushed the 27830-version branch 2 times, most recently from 627fdd0 to ff63def Compare June 8, 2017 14:24
timgraham
timgraham reviewed Jun 13, 2017
View reviewed changes
django/db/backends/postgresql/base.py Outdated
Copy link
Member

Choose a reason for hiding this comment

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

I think this renaming can be omitted.

django/contrib/gis/db/backends/postgis/operations.py Outdated
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure if this is needed. If keeping it, perhaps a test could be added.

django/contrib/gis/db/backends/spatialite/operations.py Outdated
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure if this is needed. If keeping it, perhaps a test could be added.

django/contrib/gis/geos/__init__.py Outdated
Copy link
Member

Choose a reason for hiding this comment

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

Not sure if this change is needed -- doesn't seem like Django is importing from here?

django/contrib/gis/geos/libgeos.py Outdated
Copy link
Member

Choose a reason for hiding this comment

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

get_version_tuple(geos_version_info()['version']) is okay

django/utils/version.py Outdated
Copy link
Member

Choose a reason for hiding this comment

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

"from VERSION if present" seems inaccurate

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Does Return available versions (major, minor, patch) from VERSION sound accurate?

Copy link
Member

Choose a reason for hiding this comment

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

Why is VERSION capitalized? Maybe something like:

Return a tuple of version numbers (e.g. (1, 2, 3)) from the version string (e.g. '1.2.3').

Copy link
Contributor Author

Choose a reason for hiding this comment

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

VERSION is capitalized in all other doc strings from this file. So, I thought it should be.

tests/backends/tests.py Outdated
Copy link
Member

Choose a reason for hiding this comment

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

Switching to assertTrue() seems incorrect.

tests/gis_tests/tests.py Outdated
Copy link
Member

Choose a reason for hiding this comment

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

git?

@ChillarAnand ChillarAnand force-pushed the 27830-version branch 4 times, most recently from dbe7a9e to 1799326 Compare June 13, 2017 13:54
@timgraham timgraham changed the title Fixed #27830 -- Used packaging.version.Version for version handling Fixed #27830 -- Used distutils.version.LooseVersion for version parsing. Jun 13, 2017
@timgraham timgraham merged commit 08bda82 into django:master Jun 13, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@timgraham timgraham timgraham left review comments

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

Successfully merging this pull request may close these issues.
2 participants
@ChillarAnand @timgraham