msg62571 - (view) |
Author: Benjamin Peterson (benjamin.peterson) * |
Date: 2008-02-19 21:56 |
This patch changes os.environ to inherit builtin dict rather than UserDict.
|
msg62572 - (view) |
Author: Martin v. Löwis (loewis) * |
Date: 2008-02-19 22:11 |
What's the rationale for this change?
|
msg62573 - (view) |
Author: Benjamin Peterson (benjamin.peterson) * |
Date: 2008-02-19 22:37 |
PEP 390. It's cleaner and faster.
|
msg62582 - (view) |
Author: Martin v. Löwis (loewis) * |
Date: 2008-02-20 03:51 |
I still must be missing something. There is no PEP 390, AFAICT.
|
msg62584 - (view) |
Author: Benjamin Peterson (benjamin.peterson) * |
Date: 2008-02-20 04:04 |
Forgive me. I meant 290.
|
msg62585 - (view) |
Author: Martin v. Löwis (loewis) * |
Date: 2008-02-20 04:12 |
Sorry, it still makes no sense. Up to r56113, PEP 290 doesn't seem to
talk about UserDict, os.environ, or inheritance from new-style classes.
What section are you specifically referring to?
|
msg62593 - (view) |
Author: Benjamin Peterson (benjamin.peterson) * |
Date: 2008-02-20 12:55 |
I know that it doesn't mention inheriting dict specifically, but you
asked why, so I was referring to the Rationale: "Modernization options
arise when new versions of Python add features that allow improved
clarity or higher performance than previously available." Sorry for the
confusion.
|
msg62594 - (view) |
Author: Virgil Dupras (vdupras) |
Date: 2008-02-20 16:36 |
The performance gain is rather good:
hsoft-dev:python hsoft$ ./python.exe -m "timeit" -s "import os"
"os.environ['HOME']"
1000000 loops, best of 3: 1.16 usec per loop
hsoft-dev:python hsoft$ patch -p0 < environ-modern.diff
patching file Lib/os.py
hsoft-dev:python hsoft$ ./python.exe -m "timeit" -s "import os"
"os.environ['HOME']"
1000000 loops, best of 3: 0.428 usec per loop
The regression tests still pass, and modernization of the code is a nice
thing.
+1
|
msg62824 - (view) |
Author: Alexander Belopolsky (belopolsky) * |
Date: 2008-02-23 22:33 |
Small comment on the patch:
def clear(self):
- for key in self.data.keys():
+ for key in self.keys():
unsetenv(key)
- del self.data[key]
+ del self[key]
It looks like the patched version will unsetenv twice, Change
del self[key]
to
dict.__delitem__(self, key)
+1
|
msg62827 - (view) |
Author: Benjamin Peterson (benjamin.peterson) * |
Date: 2008-02-23 22:43 |
Here's a corrected version.
|
msg62828 - (view) |
Author: Georg Brandl (georg.brandl) * |
Date: 2008-02-23 22:46 |
See #1367711 for a discussion whether this is really the way to go.
|
msg62829 - (view) |
Author: Benjamin Peterson (benjamin.peterson) * |
Date: 2008-02-23 22:49 |
And another.
|
msg62852 - (view) |
Author: Alexander Belopolsky (belopolsky) * |
Date: 2008-02-24 00:19 |
> dict doesn't dynamically bind any calls; to
> properly replace the semantics of dict, you
> have to implement *all* API.
What was the rationale for this decision? To me it looks like a hold-
over from the time when dicts were not subclassable.
I agree, this is a topic for python-ideas rather than bug-track, but if
you could point me to any prior discussions on this issue, it would help
me to either formulate a specific proposal or drop the issue before more
electrons are sacrificed.
|
msg62854 - (view) |
Author: Georg Brandl (georg.brandl) * |
Date: 2008-02-24 00:27 |
> What was the rationale for this decision? To me it looks like a hold-
> over from the time when dicts were not subclassable.
I reckon it's faster this way, and you want basic datatypes like dict to
be fast.
|
msg62857 - (view) |
Author: Martin v. Löwis (loewis) * |
Date: 2008-02-24 00:38 |
> What was the rationale for this decision? To me it looks like a hold-
> over from the time when dicts were not subclassable.
>
> I agree, this is a topic for python-ideas rather than bug-track, but if
> you could point me to any prior discussions on this issue, it would help
> me to either formulate a specific proposal or drop the issue before more
> electrons are sacrificed.
Can't find the original discussion right now, but one statement is at
http://www.mailinglistarchive.com/python-dev@python.org/msg01728.html
In essence, inheriting from dict is intentionally unspecified: if you
only change some methods, but not all, the behavior of the
non-overridden methods is unspecified. This is necessary because
a) specifying it would be a lot of work, and
b) specifying it might unreasonable restrict future implementations, and
c) specifying late binding for some of the methods would unreasonably
slow down their implementation, even if the common case that dict is
not subclassed.
|
msg62858 - (view) |
Author: Raymond Hettinger (rhettinger) * |
Date: 2008-02-24 00:51 |
The builtins make direct calls to their own internal methods. This has
existed for a long time. It would be hard to change without crushing
performance. Changing it violates Meyer's open-closed principle. And
changing it also introduces weird, unexpected behaviors (i.e. a
seemingly innocent method override can break pickling for an object --
we had a bug report on this once for dicts and sets). Also, making
these kind of changes makes it *much* harder for a builtin to guarantee
that it invariants are maintained and opening the door for segfaults.
|
msg62859 - (view) |
Author: Alexander Belopolsky (belopolsky) * |
Date: 2008-02-24 01:13 |
First, if the new thread on dict behavior does not make sense, please
see discussion at issue1367711 where it started. (My mistake following
up here.)
Second, the ability to subclass built-in type is such a great feature,
that it is a shame that doing so comes with so many caveats. The way I
understand objections to issue1367711 patch is that (1) it is hard to
derive from dict properly and (2) derived classes may be broken by
future changes to dict. The second objection applies to UserDict
derivation as much as dict derivation, only UserDict is unlikely to be
touched ever in the future. As to the first objection, if the burden is
that all methods have to be overriden, let os.environ lead by example
and do it. UserDict is a kludge from the XX century, stdlib should stop
promoting its use.
Since there is so much resistance to making dict more subclass-friendly,
what would you say to reimplementing DictMixin in C as say 'dict.mixin'?
|
msg62860 - (view) |
Author: Alexander Belopolsky (belopolsky) * |
Date: 2008-02-24 01:20 |
> The builtins make direct calls to their own internal methods.
Raymond, I guess issue2067 escaped your review. :-)
|
msg62861 - (view) |
Author: Raymond Hettinger (rhettinger) * |
Date: 2008-02-24 01:35 |
Py3.0 updte: UserDict is going to survive into Py3.0 and will be moved
into the collections module. DictMixin is replaced by the abstract
base classes for mappings.
I think the discussion here has grown far beyond the original bug
report. I recommend this one be closed and that you start a thread on
python-3000 or python-dev if you want to propose altering basic APIs or
moving the ABC code into C.
I can't say that I support your "UserDict is a kludge ..." comment.
The class has its uses and the standard library will continue to use it
where appropriate.
FWIW, at one time I also thought all uses of UserDict would ultimately
be supplanted by dict subclassing, but I found that it was somewhat
difficult to remove in some circumstances and that its API had some
advantages (i.e. it's easier to write subclass code like self.data
[computedkey]=computedvalue than to dispatch to dict.__setitem__(self,
computedkey, computedvalue) -- the latter form is somewhat errorprone
when the subclass methods become more complex).
Will look at 1367711 as you suggest.
|
msg62864 - (view) |
Author: Alexander Belopolsky (belopolsky) * |
Date: 2008-02-24 02:00 |
Let's get back on-topic.
I assume you are recommending to close this issue by rejecting the
patch. I disagree. The patch can be fixed to properly override all
methods and a unit test can be added to guarantee that all dict methods
are overridden.
The patch has two distinct benefits: (1) UserDict is not loaded on
startup; and (2) os.environ is faster. IMHO, these two reasons make the
patch worth pursuing.
BTW, one of the objections based on exec .. in os.environ behavior (see http://bugs.python.org/msg49131) has become invalid since then.
Specific issues should of course be addressed.
|
msg62873 - (view) |
Author: Martin v. Löwis (loewis) * |
Date: 2008-02-24 04:18 |
I also disagree that UserDict is a clutch, and that inheriting from dict
is in any way more modern or cleaner than inheriting from UserDict.
Hence I'm rejecting this patch. To "appeal", please discuss on python-dev.
|
msg62875 - (view) |
Author: Alexander Belopolsky (belopolsky) * |
Date: 2008-02-24 04:24 |
Did anyone mention "clutch"? :-)
Oh, well, please close issue1367711 as a duplicate.
|
|
Date |
User |
Action |
Args |
2022-04-11 14:56:30 | admin | set | github: 46397 |
2008-02-24 04:24:17 | belopolsky | set | messages:
+ msg62875 |
2008-02-24 04:18:40 | loewis | set | status: open -> closed resolution: rejected messages:
+ msg62873 |
2008-02-24 02:00:07 | belopolsky | set | messages:
+ msg62864 |
2008-02-24 01:35:01 | rhettinger | set | messages:
+ msg62861 |
2008-02-24 01:20:57 | belopolsky | set | messages:
+ msg62860 |
2008-02-24 01:13:05 | belopolsky | set | messages:
+ msg62859 |
2008-02-24 00:51:25 | rhettinger | set | nosy:
+ rhettinger messages:
+ msg62858 |
2008-02-24 00:38:07 | loewis | set | messages:
+ msg62857 |
2008-02-24 00:27:20 | georg.brandl | set | messages:
+ msg62854 |
2008-02-24 00:19:00 | belopolsky | set | messages:
+ msg62852 |
2008-02-23 22:49:35 | benjamin.peterson | set | files:
+ environ-modern3.diff messages:
+ msg62829 |
2008-02-23 22:46:41 | georg.brandl | set | nosy:
+ georg.brandl messages:
+ msg62828 |
2008-02-23 22:43:23 | benjamin.peterson | set | files:
+ environ-modern2.diff keywords:
+ patch messages:
+ msg62827 |
2008-02-23 22:33:58 | belopolsky | set | nosy:
+ belopolsky messages:
+ msg62824 |
2008-02-20 16:36:39 | vdupras | set | nosy:
+ vdupras messages:
+ msg62594 |
2008-02-20 12:55:07 | benjamin.peterson | set | messages:
+ msg62593 |
2008-02-20 04:12:34 | loewis | set | messages:
+ msg62585 |
2008-02-20 04:04:23 | benjamin.peterson | set | messages:
+ msg62584 |
2008-02-20 03:51:09 | loewis | set | messages:
+ msg62582 |
2008-02-19 22:37:22 | benjamin.peterson | set | messages:
+ msg62573 |
2008-02-19 22:11:33 | loewis | set | nosy:
+ loewis messages:
+ msg62572 |
2008-02-19 21:56:51 | benjamin.peterson | create | |