bpo-31291: fix an assertion failure in zipimport.zipimporter.get_data() #3226
Conversation
| #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.
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.
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.
Format pathname.replace('/','\\') as a literal code (surround it with double backquotes). Otherwise \\ will be interpreted in reST.
Lib/test/test_zipimport.py
Outdated
| 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.
I think it would be better to add a separate method for this test.
| #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.
The current solution LGTM.
serhiy-storchaka
added
needs backport to 3.6
type-bug
brettcannon
left a comment
brettcannon
left a comment
There was a problem hiding this comment.
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.
6 participants
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