-
-
Notifications
You must be signed in to change notification settings - Fork 30.1k
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
bpo-31291: fix an assertion failure in zipimport.zipimporter.get_data() #3226
bpo-31291: fix an assertion failure in zipimport.zipimporter.get_data() #3226
Conversation
@@ -651,7 +651,8 @@ zipimport_zipimporter_get_data_impl(ZipImporter *self, PyObject *path) | |||
Py_ssize_t path_start, path_len, len; | |||
|
|||
#ifdef ALTSEP | |||
path = _PyObject_CallMethodId(path, &PyId_replace, "CC", ALTSEP, SEP); | |||
path = _PyObject_CallMethodId((PyObject *)&PyUnicode_Type, &PyId_replace, | |||
"OCC", path, ALTSEP, SEP); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure about this.
would it be better to simply check whether the return value of pathname.replace('/', '\') is a str?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The current solution LGTM.
I just rejected the issue, so closing the PR as well. Any discussion about the closure should happen on the issue. |
@@ -0,0 +1,3 @@ | |||
Fix an assertion failure in `zipimport.zipimporter.get_data` on Windows, | |||
when the return value of pathname.replace('/','\\') isn't a string. Patch by |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Format pathname.replace('/','\\')
as a literal code (surround it with double backquotes). Otherwise \\
will be interpreted in reST.
Lib/test/test_zipimport.py
Outdated
@@ -517,6 +517,12 @@ def testGetData(self): | |||
zi = zipimport.zipimporter(TEMP_ZIP) | |||
self.assertEqual(data, zi.get_data(name)) | |||
self.assertIn('zipimporter object', repr(zi)) | |||
# Issue #31291: There shouldn't be an assertion failure in |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it would be better to add a separate method for this test.
@@ -651,7 +651,8 @@ zipimport_zipimporter_get_data_impl(ZipImporter *self, PyObject *path) | |||
Py_ssize_t path_start, path_len, len; | |||
|
|||
#ifdef ALTSEP | |||
path = _PyObject_CallMethodId(path, &PyId_replace, "CC", ALTSEP, SEP); | |||
path = _PyObject_CallMethodId((PyObject *)&PyUnicode_Type, &PyId_replace, | |||
"OCC", path, ALTSEP, SEP); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The current solution LGTM.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Serhiy's arguments on the issue tracker convinced me (plus I think he knows this module better than I do now 😉 ).
BTW, I don't know if I will have time to commit this between now and taking holiday, but if no one merges this by September 15, @orenmn , just leave a comment messaging me. |
…get_data() (pythonGH-3226) if pathname.replace('/', '\\') returns non-string. (cherry picked from commit 631fdee)
GH-3243 is a backport of this pull request to the 3.6 branch. |
…ta() (python#3226) if pathname.replace('/', '\\') returns non-string.
zipimport.c
: replace the C equivalent ofpathname.replace('/', '\\')
withstr.replace(pathname, '/', '\\')
.test_zipimport.py
: add a test to verify the assertion failure is no more.https://bugs.python.org/issue31291