bpo-31109: Convert zipimport to argument clinic#2990
Conversation
jarondl
force-pushed
the
zipimport-clinic
branch
from
10e92c6 to
940ad28
Compare
Convert the zipimport module to argument clinic. Mostly straight-forward, had to rename a few argument names.
| /*[clinic input] | ||
| zipimport.zipimporter.__init__ | ||
|
|
||
| archivepath: object(converter="PyUnicode_FSDecoder") |
There was a problem hiding this comment.
Why not use just the name "path"?
There was a problem hiding this comment.
Do you mean path: or archivepath as path:?
Sure I can do the latter, but what's the benefit? It was more straight forward to rename the variable to match the signature. The meaning of archivepath is clearer
There was a problem hiding this comment.
The benefit of not renaming is the patch is much easier to review that way. As it stands it's much harder for us to check the accuracy of the changes due to so many lines being flagged as changed compared to just reviewing the Argument Clinic changes which is what this PR is specifically targeting.
| /*[clinic input] | ||
| zipimport.zipimporter.find_module | ||
|
|
||
| fullname: unicode |
There was a problem hiding this comment.
The arguments of this method (and many other methods) were positional-only. Converting to Argument Clinic shouldn't change the behavior.
There was a problem hiding this comment.
Actually all the methods here were positional-only. But this change is backwards compatible and logical. See for example http://bugs.python.org/issue20294#msg208444
Why should we force positional-only? Your patch in bpo-25711 to convert zipimport to Python would also have the same effect.
There was a problem hiding this comment.
Do you prefer to keep Argument Clinic conversion separate from behavioral changes? Then we can add trailing /s to the argument lists and remove them in a separate PR.
There was a problem hiding this comment.
Yeah, we prefer to not change semantics while doing an Argument Clinic conversion (we have run into issues in the past when doing both at the same time).
There was a problem hiding this comment.
OK, done, now they are positional only. Thank you both for reviewing.
Preserve positional only arguments in zipimport after argument clinic conversion.
| /*[clinic input] | ||
| zipimport.zipimporter.__init__ | ||
|
|
||
| archivepath: object(converter="PyUnicode_FSDecoder") |
There was a problem hiding this comment.
The benefit of not renaming is the patch is much easier to review that way. As it stands it's much harder for us to check the accuracy of the changes due to so many lines being flagged as changed compared to just reviewing the Argument Clinic changes which is what this PR is specifically targeting.
Revert the argument names archivepath and pathname to their original names in the code (path, and path).
brettcannon
left a comment
brettcannon
left a comment
There was a problem hiding this comment.
Three really minor formatting tweaks necessary and then I think this will be ready to go!
| '/tmp/myimport.zip/mydirectory', if mydirectory is a valid directory inside | ||
| the archive. | ||
|
|
||
| 'ZipImportError is raised if 'archivepath' doesn't point to a valid Zip |
There was a problem hiding this comment.
Looks like there is a stray ' at the start of this sentence.
| 'ZipImportError is raised if 'archivepath' doesn't point to a valid Zip | ||
| archive. | ||
|
|
||
| The 'archive' attribute of zipimporter objects contains the name of the |
There was a problem hiding this comment.
"zipimporter objects" -> "the zipimporter object".
|
|
||
| Search for a module specified by 'fullname'. | ||
|
|
||
| 'fullname' must be the |
brettcannon
left a comment
brettcannon
left a comment
There was a problem hiding this comment.
@serhiy-storchaka did you have any other feedback or can I merge this?
|
@serhiy-storchaka and @brettcannon , is there anything else I can do to have this merged? |
|
Since it looks like @serhiy-storchaka has nothing to add would you mind adding a news entry? See https://devguide.python.org/committing/#what-s-new-and-news-entries on how to do that. Once that's file in I can touch it up as necessary and then merge this! |
|
A Python core developer, brettcannon, has requested some changes be Once you have made the requested changes, please leave a comment |
|
Blimey, I didn't expect the Spanish Inquisition! |
|
Nobody expects the Spanish Inquisition! @brettcannon: please review the changes made to this pull request. |
brettcannon
left a comment
brettcannon
left a comment
There was a problem hiding this comment.
Once Travis goes green I'll merge!
|
Thanks for the work, @jarondl ! |
6 participants
Convert the zipimport module to argument clinic. Mostly
straight-forward, had to rename a few argument names.
https://bugs.python.org/issue31109