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

Restricting the packaging version to less than 20 #44

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

narenandu
Copy link

OS: CentOS7
Python version: Python2.7

Following would work with packaging-19.2 version when checked for typehinting using mypy

from packaging import version
version.parse(u"hello world")

But when packaging-20 is used there is an error originating from packaging saying the argument should strictly be a string (Text)

This PR is to restrict the packaging version to packaging-19, since mypy checks break automatically with packaging-20, which was released on Jan 5th 2020 (https://pypi.org/project/packaging/20.0/)

which changed the argument type for version.parse to be
strictly string rather than flexible with unicode
@codecov-io
Copy link

Codecov Report

Merging #44 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@          Coverage Diff          @@
##           master    #44   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files           1      1           
  Lines          83     83           
  Branches       16     16           
=====================================
  Hits           83     83

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 442a041...0d99039. Read the comment docs.

@briancurtin
Copy link
Owner

Do you have a test case which shows that this would fail? Locally, the test suite passes with packaging==20.0.

I'm not sure restricting the packaging version is the right move instead of fixing whatever the problem is, so I think it'd be helpful to see how code using deprecation fails with it so that we can come up with a fix.

@narenandu
Copy link
Author

@briancurtin , unless the tests are using mypy in someway to check for type hints, you won't be able to see the issue which I am trying to fix. But following is the output from mypy.

: error: Argument 1 to "parse" has incompatible type "unicode"; expected "str"

If you want to see the issue, please follow the steps (assuming a linux platform)

virtualenv test_case
. test_case/bin/activate
pip install packaging==20.0 mypy   # assuming you are using python3 as your /usr/bin/python
touch test.py

Then paste the content in test.py

from packaging import version
version.parse(u"hello world")

Run mypy on the file

mypy test.py

and should see the following error

: error: Argument 1 to "parse" has incompatible type "unicode"; expected "str"

But with packaging version restricted to under 20, there is no mypy error, which tells me that between the major versions of 19 and 20 of packaging, the typehints has been changed for the packaging.version.parse. Since it a breaking release (major version) from 19 to 20, thought it would nice to restrict the version of packaging.

I am open for any ideas as to how we can tackle this !

@narenandu
Copy link
Author

FYI: https://github.com/pypa/packaging/releases/tag/20.0

Properly mark packaging has being fully typed by adding a py.typed file (:issue:226)
Add type hints (:issue:191)

https://github.com/pypa/packaging/pull/200/files#diff-832b3493370c7de79eff39a50afebf2aR49 is the change which is making the argument mandatory str

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.

3 participants