Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

os.environ should inherit dict #46397

Closed
benjaminp opened this issue Feb 19, 2008 · 22 comments
Closed

os.environ should inherit dict #46397

benjaminp opened this issue Feb 19, 2008 · 22 comments
Labels
type-feature A feature request or enhancement

Comments

@benjaminp
Copy link
Contributor

BPO 2144
Nosy @loewis, @birkenfeld, @rhettinger, @abalkin, @benjaminp
Files
  • environ-modern.diff
  • environ-modern2.diff
  • environ-modern3.diff
  • Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.

    Show more details

    GitHub fields:

    assignee = None
    closed_at = <Date 2008-02-24.04:18:40.982>
    created_at = <Date 2008-02-19.21:56:51.678>
    labels = ['type-feature']
    title = 'os.environ should inherit dict'
    updated_at = <Date 2008-02-24.04:24:17.417>
    user = 'https://github.com/benjaminp'

    bugs.python.org fields:

    activity = <Date 2008-02-24.04:24:17.417>
    actor = 'belopolsky'
    assignee = 'none'
    closed = True
    closed_date = <Date 2008-02-24.04:18:40.982>
    closer = 'loewis'
    components = []
    creation = <Date 2008-02-19.21:56:51.678>
    creator = 'benjamin.peterson'
    dependencies = []
    files = ['9466', '9524', '9525']
    hgrepos = []
    issue_num = 2144
    keywords = ['patch']
    message_count = 22.0
    messages = ['62571', '62572', '62573', '62582', '62584', '62585', '62593', '62594', '62824', '62827', '62828', '62829', '62852', '62854', '62857', '62858', '62859', '62860', '62861', '62864', '62873', '62875']
    nosy_count = 6.0
    nosy_names = ['loewis', 'georg.brandl', 'rhettinger', 'belopolsky', 'benjamin.peterson', 'vdupras']
    pr_nums = []
    priority = 'normal'
    resolution = 'rejected'
    stage = None
    status = 'closed'
    superseder = None
    type = 'enhancement'
    url = 'https://bugs.python.org/issue2144'
    versions = ['Python 2.6']

    @benjaminp
    Copy link
    Contributor Author

    This patch changes os.environ to inherit builtin dict rather than UserDict.

    @benjaminp benjaminp added the type-feature A feature request or enhancement label Feb 19, 2008
    @loewis
    Copy link
    Mannequin

    loewis mannequin commented Feb 19, 2008

    What's the rationale for this change?

    @benjaminp
    Copy link
    Contributor Author

    PEP-390. It's cleaner and faster.

    @loewis
    Copy link
    Mannequin

    loewis mannequin commented Feb 20, 2008

    I still must be missing something. There is no PEP-390, AFAICT.

    @benjaminp
    Copy link
    Contributor Author

    Forgive me. I meant 290.

    @loewis
    Copy link
    Mannequin

    loewis mannequin commented Feb 20, 2008

    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?

    @benjaminp
    Copy link
    Contributor Author

    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.

    @vdupras
    Copy link
    Mannequin

    vdupras mannequin commented Feb 20, 2008

    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

    @abalkin
    Copy link
    Member

    abalkin commented Feb 23, 2008

    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

    @benjaminp
    Copy link
    Contributor Author

    Here's a corrected version.

    @birkenfeld
    Copy link
    Member

    See bpo-1367711 for a discussion whether this is really the way to go.

    @benjaminp
    Copy link
    Contributor Author

    And another.

    @abalkin
    Copy link
    Member

    abalkin commented Feb 24, 2008

    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.

    @birkenfeld
    Copy link
    Member

    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.

    @loewis
    Copy link
    Mannequin

    loewis mannequin commented Feb 24, 2008

    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.

    @rhettinger
    Copy link
    Contributor

    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.

    @abalkin
    Copy link
    Member

    abalkin commented Feb 24, 2008

    First, if the new thread on dict behavior does not make sense, please
    see discussion at bpo-1367711 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 bpo-1367711 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'?

    @abalkin
    Copy link
    Member

    abalkin commented Feb 24, 2008

    The builtins make direct calls to their own internal methods.

    Raymond, I guess bpo-2067 escaped your review. :-)

    @rhettinger
    Copy link
    Contributor

    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.

    @abalkin
    Copy link
    Member

    abalkin commented Feb 24, 2008

    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.

    @loewis
    Copy link
    Mannequin

    loewis mannequin commented Feb 24, 2008

    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.

    @loewis loewis mannequin closed this as completed Feb 24, 2008
    @abalkin
    Copy link
    Member

    abalkin commented Feb 24, 2008

    Did anyone mention "clutch"? :-)

    Oh, well, please close bpo-1367711 as a duplicate.

    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    type-feature A feature request or enhancement
    Projects
    None yet
    Development

    No branches or pull requests

    4 participants