Title: importlib's __import__() argument style nit
Type: behavior Stage: commit review
Components: Library (Lib) Versions: Python 3.3
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: brett.cannon Nosy List: Arfrever, barry, brett.cannon, georg.brandl, meador.inge, python-dev
Priority: release blocker Keywords: easy, patch

Created on 2012-07-27 17:09 by barry, last changed 2012-08-06 21:35 by barry. This issue is now closed.

File name Uploaded Description Edit
issue-15471.patch meador.inge, 2012-07-30 04:39 review
immutable_defaults_for_import.diff brett.cannon, 2012-08-05 23:50 review
Messages (8)
msg166585 - (view) Author: Barry A. Warsaw (barry) * (Python committer) Date: 2012-07-27 17:09
Very minor style nit.  In general, it's not considered good style to use mutable objects in default argument values.  importlib's _bootstrap.__import__() does just this for its globals, locals, and fromlist arguments.

The implementation currently doesn't abuse this, or allow any of the extensions to abuse, it may be possible in the future to naively cause negative side-effects due to mutate the keyword arguments.  It would be better to use non-mutable default values in the argument list.
msg166856 - (view) Author: Meador Inge (meador.inge) * (Python committer) Date: 2012-07-30 04:39
How about the attached?
msg167064 - (view) Author: Barry A. Warsaw (barry) * (Python committer) Date: 2012-07-31 22:56
On Jul 30, 2012, at 04:39 AM, Meador Inge wrote:

>Meador Inge added the comment:
>How about the attached?

What about something like:

    globals = ({} if globals is None else globals)

and similarly for locals?
msg167523 - (view) Author: Brett Cannon (brett.cannon) * (Python committer) Date: 2012-08-05 23:50
Attached is a patch that drops all mutable default arguments to __import__() and updates the docs. I went with Barry's approach but made it compatible with PEP 8 (bad, FLUFL; no unneeded parens!). I also ignored the locals argument since __import__ doesn't even use it.

Assigning to Georg as a release blocker to make a call as to whether I can apply this to 3.3 of if I need to wait until 3.4 since this is not a critical fix but a good thing to do. Just assign back to me once the decision has been made.
msg167536 - (view) Author: Meador Inge (meador.inge) * (Python committer) Date: 2012-08-06 04:59
msg167539 - (view) Author: Georg Brandl (georg.brandl) * (Python committer) Date: 2012-08-06 05:35
Looks good and safe to me. +1.
msg167578 - (view) Author: Roundup Robot (python-dev) Date: 2012-08-06 20:34
New changeset 4240282a9f4a by Brett Cannon in branch 'default':
Issue #15471: Don't use mutable object as default values for the
msg167587 - (view) Author: Barry A. Warsaw (barry) * (Python committer) Date: 2012-08-06 21:35
On Aug 05, 2012, at 11:50 PM, Brett Cannon wrote:

>I went with Barry's approach but made it compatible with PEP 8 (bad, FLUFL;
>no unneeded parens!).

I actually think I picked that up from the big guy himself, but I could be
misremembering. ;)
Date User Action Args
2012-08-06 21:35:34barrysetmessages: + msg167587
2012-08-06 20:35:12brett.cannonsetstatus: open -> closed
resolution: fixed
2012-08-06 20:34:54python-devsetnosy: + python-dev
messages: + msg167578
2012-08-06 05:35:46georg.brandlsetassignee: georg.brandl -> brett.cannon
messages: + msg167539
2012-08-06 04:59:52meador.ingesetmessages: + msg167536
2012-08-06 00:49:48Arfreversetnosy: + Arfrever
2012-08-05 23:50:17brett.cannonsetfiles: + immutable_defaults_for_import.diff
priority: low -> release blocker

assignee: brett.cannon -> georg.brandl

nosy: + georg.brandl
messages: + msg167523
stage: patch review -> commit review
2012-07-31 22:56:46barrysetmessages: + msg167064
2012-07-30 04:39:45meador.ingesetfiles: + issue-15471.patch

type: behavior

keywords: + patch
nosy: + meador.inge
messages: + msg166856
stage: patch review
2012-07-27 17:09:37barrycreate