bpo-31109: Convert zipimport to argument clinic #2990

jarondl · 2017-08-02T19:06:14Z

Convert the zipimport module to argument clinic. Mostly
straight-forward, had to rename a few argument names.

https://bugs.python.org/issue31109

Convert the zipimport module to argument clinic. Mostly straight-forward, had to rename a few argument names.

serhiy-storchaka · 2017-08-03T07:11:00Z

Modules/zipimport.c

+/*[clinic input]
+zipimport.zipimporter.__init__
+
+    archivepath: object(converter="PyUnicode_FSDecoder")


Why not use just the name "path"?

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

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.

serhiy-storchaka · 2017-08-03T07:12:19Z

Modules/zipimport.c

+/*[clinic input]
+zipimport.zipimporter.find_module
+
+    fullname: unicode


The arguments of this method (and many other methods) were positional-only. Converting to Argument Clinic shouldn't change the behavior.

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.

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.

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).

OK, done, now they are positional only. Thank you both for reviewing.

Preserve positional only arguments in zipimport after argument clinic conversion.

brettcannon · 2017-08-09T18:40:04Z

Modules/zipimport.c

+/*[clinic input]
+zipimport.zipimporter.__init__
+
+    archivepath: object(converter="PyUnicode_FSDecoder")


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

Three really minor formatting tweaks necessary and then I think this will be ready to go!

brettcannon · 2017-08-10T19:50:41Z

Modules/zipimport.c

+'/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


Looks like there is a stray ' at the start of this sentence.

brettcannon · 2017-08-10T19:51:19Z

Modules/zipimport.c

+'ZipImportError is raised if 'archivepath' doesn't point to a valid Zip
+archive.
+
+The 'archive' attribute of zipimporter objects contains the name of the


"zipimporter objects" -> "the zipimporter object".

brettcannon · 2017-08-10T19:54:59Z

Modules/zipimport.c

+
+Search for a module specified by 'fullname'.
+
+'fullname' must be the


Premature line break.

brettcannon

@serhiy-storchaka did you have any other feedback or can I merge this?

jarondl · 2017-08-17T00:45:45Z

@serhiy-storchaka and @brettcannon , is there anything else I can do to have this merged?
In any case I want to say thank you both for the reviews.

brettcannon · 2017-08-17T19:08:41Z

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!

brettcannon

Please add a news entry.

bedevere-bot · 2017-08-17T19:09:08Z

A Python core developer, brettcannon, has requested some changes be
made to your pull request before we can consider merging it. If you
could please address their requests along with any other requests in
other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment
on this pull request containing the phrase I didn't expect the Spanish Inquisition!
I will then notify brettcannon along with any other core developers
who have left a review that you're ready for them to take another look
at this pull request.

jarondl · 2017-08-18T00:35:28Z

Blimey, I didn't expect the Spanish Inquisition!

bedevere-bot · 2017-08-18T00:35:30Z

Nobody expects the Spanish Inquisition!

@brettcannon: please review the changes made to this pull request.

brettcannon

Once Travis goes green I'll merge!

brettcannon · 2017-08-18T18:41:27Z

Thanks for the work, @jarondl !

jarondl requested a review from brettcannon as a code owner August 2, 2017 19:06

the-knights-who-say-ni added the CLA signed label Aug 2, 2017

jarondl force-pushed the zipimport-clinic branch from 10e92c6 to 940ad28 Compare August 2, 2017 19:08

bpo-31109: Convert zipimport to argument clinic

940ad28

Convert the zipimport module to argument clinic. Mostly straight-forward, had to rename a few argument names.

serhiy-storchaka reviewed Aug 3, 2017

View reviewed changes

bpo-31109: zipimport argument clinic: positional only

ed493fa

Preserve positional only arguments in zipimport after argument clinic conversion.

brettcannon requested changes Aug 9, 2017

View reviewed changes

bpo-31109: zipimport argument clinic: revert arg names

e5544ca

Revert the argument names archivepath and pathname to their original names in the code (path, and path).

brettcannon requested changes Aug 10, 2017

View reviewed changes

bpo-31109: zipimporter docstrings minor formatting fixes

46b86d9

brettcannon approved these changes Aug 10, 2017

View reviewed changes

brettcannon self-assigned this Aug 17, 2017

brettcannon added the awaiting changes label Aug 17, 2017

brettcannon requested changes Aug 17, 2017

View reviewed changes

bpo-31109: add NEWS entry

b22cc75

bedevere-bot added awaiting change review and removed awaiting changes labels Aug 18, 2017

Very minor news entry tweak

4f250ab

brettcannon approved these changes Aug 18, 2017

View reviewed changes

bedevere-bot added awaiting merge and removed awaiting change review labels Aug 18, 2017

brettcannon merged commit 02f3b7d into python:master Aug 18, 2017

Mariatta removed the awaiting merge label Aug 29, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

bpo-31109: Convert zipimport to argument clinic #2990

bpo-31109: Convert zipimport to argument clinic #2990

jarondl commented Aug 2, 2017 •

edited by bedevere-bot

Loading

serhiy-storchaka Aug 3, 2017

jarondl Aug 3, 2017 •

edited

Loading

brettcannon Aug 9, 2017

jarondl Aug 10, 2017

serhiy-storchaka Aug 3, 2017

jarondl Aug 3, 2017

jarondl Aug 3, 2017

brettcannon Aug 3, 2017

jarondl Aug 3, 2017

brettcannon Aug 9, 2017

brettcannon left a comment

brettcannon Aug 10, 2017

brettcannon Aug 10, 2017

brettcannon Aug 10, 2017

brettcannon left a comment

jarondl commented Aug 17, 2017

brettcannon commented Aug 17, 2017

brettcannon left a comment

bedevere-bot commented Aug 17, 2017

jarondl commented Aug 18, 2017

bedevere-bot commented Aug 18, 2017

brettcannon left a comment

brettcannon commented Aug 18, 2017


		Search for a module specified by 'fullname'.

		'fullname' must be the

bpo-31109: Convert zipimport to argument clinic #2990

bpo-31109: Convert zipimport to argument clinic #2990

Conversation

jarondl commented Aug 2, 2017 • edited by bedevere-bot Loading

Choose a reason for hiding this comment

jarondl Aug 3, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brettcannon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brettcannon left a comment

Choose a reason for hiding this comment

jarondl commented Aug 17, 2017

brettcannon commented Aug 17, 2017

brettcannon left a comment

Choose a reason for hiding this comment

bedevere-bot commented Aug 17, 2017

jarondl commented Aug 18, 2017

bedevere-bot commented Aug 18, 2017

brettcannon left a comment

Choose a reason for hiding this comment

brettcannon commented Aug 18, 2017

jarondl commented Aug 2, 2017 •

edited by bedevere-bot

Loading

jarondl Aug 3, 2017 •

edited

Loading