bpo-30274: Rename 'name' to 'fullname' argument to ExtensionFileLoader. by sayanchowdhury · Pull Request #1735 · python/cpython

sayanchowdhury · 2017-05-22T23:30:08Z

DeprecationWarning is raised for any use of 'name'.

https://bugs.python.org/issue30274

Automerge-Triggered-By: @brettcannon

ambv · 2017-05-22T23:32:26Z

You also need to use the deprecated name if it's given.

ambv · 2017-05-23T05:35:18Z

You need to put self.name = fullname in an else: statement, otherwise you'll always re-assign self.name to fullname anyway.

Oops, terrible mistake it was. Fixed it.

Mariatta · 2017-06-12T06:50:55Z

I believe this was done during Pycon US sprint, so I applied the sprint label.

berkerpeksag · 2017-07-23T23:53:35Z

You need to pass stacklevel=2 in order to make DeprecationWarning more useful.

Also, please add a test case for DeprecationWarning.

berkerpeksag · 2017-07-23T23:55:32Z

We don't edit Misc/NEWS manually anymore. Please use blurb to create a NEWS entry: https://github.com/python/core-workflow/tree/master/blurb

Okay, I will looking in blurb and update the PR accordingly

serhiy-storchaka

The side effect of this change is that it makes both arguments optional, with default to None. I have doubts that this is an intentional change.

The only correct calls are:

ExtensionFileLoader(thename, thepath)
ExtensionFileLoader(thename, path=thepath)
ExtensionFileLoader(fullname=thename, path=thepath)
ExtensionFileLoader(name=thename, path=thepath) # deprecated

Passing both fullname and name should be an error.

bedevere-bot · 2017-11-02T20:44:24Z

A Python core developer 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 have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

brettcannon · 2018-07-20T23:16:29Z

@sayanchowdhury do you plan to address the review comments or should I close it?

sayanchowdhury · 2018-07-24T21:57:17Z

@brettcannon I'll try to get this closed as soon as possible.

sayanchowdhury · 2018-10-13T23:05:06Z

@serhiy-storchaka Which error should I be throwing here? ValueError?

serhiy-storchaka · 2018-10-14T04:09:51Z

TypeError, as in the case when you pass positional and keyword argument for the same parameter.

sayanchowdhury · 2018-10-14T13:55:58Z

Though I've written the test for DeprecationWarning I don't know if it's correct. Locally, my tests are failing with this error

======================================================================
FAIL: test_name_deprecation (test.test_importlib.extension.test_loader.Frozen_LoaderTests)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/sachowdh/code/cpython/Lib/test/test_importlib/extension/test_loader.py", line 34, in test_name_deprecation
    other = self.machinery.ExtensionFileLoader(
AssertionError: DeprecationWarning not triggered

----------------------------------------------------------------------

I tried to import ExtensionFileLoader in Python shell and it does not reflect the updated arguments i.e. fullname. How can I fix it?

➜  cpython git:(bpo30274) ✗ ./python                                                     
Python 3.8.0a0 (heads/bpo30274-dirty:8e5ac1790b, Oct 14 2018, 19:03:35) 
[GCC 8.1.1 20180712 (Red Hat 8.1.1-5)] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> from importlib.machinery import ExtensionFileLoader
>>> help(ExtensionFileLoader)

class ExtensionFileLoader(FileLoader, _LoaderBasics)
 |  ExtensionFileLoader(name, path)

berkerpeksag · 2018-10-16T18:59:04Z

@sayanchowdhury did you run make regen-importlib?

sayanchowdhury · 2018-10-17T10:20:32Z

@berkerpeksag I did not. Thanks. I have updated the PR.

I don't know if the message for TypeError sounds good. Please review the PR.

berkerpeksag

Please add a NEWS entry. See https://devguide.python.org/committing/#what-s-new-and-news-entries for details.

bedevere-bot · 2020-03-20T08:09:39Z

Thanks for making the requested changes!

@berkerpeksag, @serhiy-storchaka, @brettcannon, @ambv: please review the changes made to this pull request.

brettcannon

I'm really sorry to send this back to you, @sayanchowdhury, but I just noticed some indentation inconsistencies. (I would have committed them myself and then merged but I don't think you checked the box to let me edit the PR).

bedevere-bot · 2020-03-20T22:49:40Z

A Python core developer 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 have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

Signed-off-by: Sayan Chowdhury <sayan.chowdhury2012@gmail.com>

sayanchowdhury · 2020-03-21T16:36:31Z

I'm really sorry to send this back to you, @sayanchowdhury, but I just noticed some indentation inconsistencies. (I would have committed them myself and then merged but I don't think you checked the box to let me edit the PR).

Sorry for that one. Probably because it is an old PR.

sayanchowdhury · 2020-03-21T16:36:41Z

I have made the requested changes; please review again.

bedevere-bot · 2020-03-21T16:36:46Z

Thanks for making the requested changes!

@berkerpeksag, @serhiy-storchaka, @brettcannon, @ambv: please review the changes made to this pull request.

serhiy-storchaka · 2020-03-22T09:12:55Z

👍

brettcannon · 2020-03-25T17:31:59Z

@sayanchowdhury changes looks great! Unfortunately the macOS tests failed for some reason and now there's a merge conflict. If you could fix the merge conflict then hopefully the tests will pass and then we can merge this!

miss-islington · 2020-03-25T17:32:15Z

Sorry, I can't merge this PR. Reason: Pull Request is not mergeable.

csabella · 2020-05-23T01:00:35Z

@sayanchowdhury, please resolve the merge conflict as this has been approved and is ready to merge. Thanks!

DimitrisJim · 2021-05-17T10:59:37Z

Ping. I see this has stalled, in what way can we get it started again?

brettcannon · 2021-05-17T22:39:19Z

@DimitrisJim macOS test failure needs to be resolved and the merge conflict needs to be fixed.

github-actions · 2022-02-19T00:08:36Z

This PR is stale because it has been open for 30 days with no activity.

serhiy-storchaka

The problem is that it makes all parameters optional. ExtensionFileLoader() is now valid.

brettcannon · 2022-05-04T20:13:56Z

At this point I think the risk of breakage outweighs the purity of normalizing the argument name. Thanks to @sayanchowdhury for the PR, but I think this has stalled out too long to merge at this point.

the-knights-who-say-ni added the CLA signed label May 22, 2017

sayanchowdhury changed the title ~~bpo30274: Rename 'name' to 'fullname' argument to ExtensionFileLoader.~~ bpo-30274: Rename 'name' to 'fullname' argument to ExtensionFileLoader. May 22, 2017

ambv requested changes May 22, 2017

View reviewed changes

Comment thread Lib/importlib/_bootstrap_external.py Outdated

Copy link
Copy Markdown

Contributor

ambv May 22, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You also need to use the deprecated name if it's given.

sayanchowdhury force-pushed the bpo30274 branch from 011bc46 to 260057d Compare May 23, 2017 00:19

ambv requested changes May 23, 2017

View reviewed changes

sayanchowdhury force-pushed the bpo30274 branch from 260057d to f54b116 Compare May 23, 2017 17:41

Mariatta added the sprint label Jun 12, 2017

berkerpeksag requested changes Jul 23, 2017

View reviewed changes

Mariatta added needs rebase and removed needs rebase labels Oct 9, 2017

sayanchowdhury force-pushed the bpo30274 branch from f54b116 to 9aee00f Compare October 29, 2017 06:23

sayanchowdhury requested a review from a team October 29, 2017 06:23

sayanchowdhury force-pushed the bpo30274 branch from 9aee00f to 340ba58 Compare November 2, 2017 10:53

serhiy-storchaka requested changes Nov 2, 2017

View reviewed changes

bedevere-bot added the awaiting changes label Nov 2, 2017

sayanchowdhury force-pushed the bpo30274 branch from 340ba58 to 8e5ac17 Compare October 13, 2018 22:48

sayanchowdhury force-pushed the bpo30274 branch from bf2ad7b to 882bc02 Compare October 17, 2018 10:17

berkerpeksag requested changes Oct 17, 2018

View reviewed changes

Comment thread Lib/test/test_importlib/extension/test_loader.py Outdated

Comment thread Lib/test/test_importlib/extension/test_loader.py Outdated

sayanchowdhury force-pushed the bpo30274 branch from 882bc02 to 1d03f9e Compare October 17, 2018 11:14

bedevere-bot requested a review from brettcannon March 20, 2020 08:09

brettcannon requested changes Mar 20, 2020

View reviewed changes

Comment thread Lib/test/test_importlib/extension/test_loader.py Outdated

Comment thread Lib/test/test_importlib/extension/test_loader.py Outdated

bedevere-bot added awaiting changes and removed awaiting change review labels Mar 20, 2020

brettcannon reviewed Mar 20, 2020

View reviewed changes

Comment thread Doc/whatsnew/3.9.rst Outdated

Fix the indentation inconsistencies and update doc

c7b25cf

Signed-off-by: Sayan Chowdhury <sayan.chowdhury2012@gmail.com>

sayanchowdhury force-pushed the bpo30274 branch from b6d8d63 to c7b25cf Compare March 21, 2020 16:00

bedevere-bot added awaiting change review and removed awaiting changes labels Mar 21, 2020

bedevere-bot requested a review from brettcannon March 21, 2020 16:36

serhiy-storchaka approved these changes Mar 22, 2020

View reviewed changes

bedevere-bot removed the awaiting change review label Mar 22, 2020

brettcannon approved these changes Mar 25, 2020

View reviewed changes

zkneupper mentioned this pull request May 17, 2021

bpo-30274: Rename 'name' to 'fullname' argument to ExtensionFileLoader. #26195

Closed

achhina mentioned this pull request May 4, 2022

GH-74460: Renamed name argument to fullname for importlib.machinery.ExtensionFileLoader #92300

Closed

4 tasks

serhiy-storchaka reviewed May 4, 2022

View reviewed changes

brettcannon mentioned this pull request May 4, 2022

Rename 'name' to 'fullname' argument to ExtensionFileLoader #74460

Closed

Uh oh!

Conversation

sayanchowdhury commented May 22, 2017 • edited by miss-islington Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ambv May 22, 2017

Choose a reason for hiding this comment

Uh oh!

ambv May 23, 2017

Choose a reason for hiding this comment

Uh oh!

sayanchowdhury May 23, 2017

Choose a reason for hiding this comment

Uh oh!

Mariatta commented Jun 12, 2017

Uh oh!

berkerpeksag Jul 23, 2017

Choose a reason for hiding this comment

Uh oh!

berkerpeksag Jul 23, 2017

Choose a reason for hiding this comment

Uh oh!

berkerpeksag Jul 23, 2017

Choose a reason for hiding this comment

Uh oh!

sayanchowdhury Jul 24, 2017

Choose a reason for hiding this comment

Uh oh!

serhiy-storchaka left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bedevere-bot commented Nov 2, 2017

Uh oh!

brettcannon commented Jul 20, 2018

Uh oh!

sayanchowdhury commented Jul 24, 2018

Uh oh!

sayanchowdhury commented Oct 13, 2018

Uh oh!

serhiy-storchaka commented Oct 14, 2018 • edited by brettcannon Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sayanchowdhury commented Oct 14, 2018

Uh oh!

berkerpeksag commented Oct 16, 2018

Uh oh!

sayanchowdhury commented Oct 17, 2018

Uh oh!

berkerpeksag left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

bedevere-bot commented Mar 20, 2020

Uh oh!

brettcannon left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

bedevere-bot commented Mar 20, 2020

Uh oh!

Uh oh!

sayanchowdhury commented Mar 21, 2020

Uh oh!

sayanchowdhury commented Mar 21, 2020

Uh oh!

bedevere-bot commented Mar 21, 2020

Uh oh!

serhiy-storchaka commented Mar 22, 2020

Uh oh!

brettcannon commented Mar 25, 2020

Uh oh!

miss-islington commented Mar 25, 2020

Uh oh!

csabella commented May 23, 2020

Uh oh!

DimitrisJim commented May 17, 2021

Uh oh!

sayanchowdhury commented May 22, 2017 •

edited by miss-islington

Loading

serhiy-storchaka left a comment •

edited

Loading

serhiy-storchaka commented Oct 14, 2018 •

edited by brettcannon

Loading