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

ChainMap.new_child could use improvement #60817

Closed
vsajip opened this issue Dec 5, 2012 · 9 comments
Closed

ChainMap.new_child could use improvement #60817

vsajip opened this issue Dec 5, 2012 · 9 comments
Assignees
Labels
stdlib Python modules in the Lib dir type-feature A feature request or enhancement

Comments

@vsajip
Copy link
Member

vsajip commented Dec 5, 2012

BPO 16613
Nosy @doerwalter, @rhettinger, @vsajip, @bitdancer
Files
  • new-child.diff: Code, test, and doc change for feature.
  • 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 = 'https://github.com/vsajip'
    closed_at = <Date 2013-01-11.23:40:07.204>
    created_at = <Date 2012-12-05.09:13:11.098>
    labels = ['type-feature', 'library']
    title = 'ChainMap.new_child could use improvement'
    updated_at = <Date 2013-01-11.23:40:07.203>
    user = 'https://github.com/vsajip'

    bugs.python.org fields:

    activity = <Date 2013-01-11.23:40:07.203>
    actor = 'python-dev'
    assignee = 'vinay.sajip'
    closed = True
    closed_date = <Date 2013-01-11.23:40:07.204>
    closer = 'python-dev'
    components = ['Library (Lib)']
    creation = <Date 2012-12-05.09:13:11.098>
    creator = 'vinay.sajip'
    dependencies = []
    files = ['28694']
    hgrepos = ['171']
    issue_num = 16613
    keywords = ['patch']
    message_count = 9.0
    messages = ['176974', '179307', '179552', '179563', '179671', '179674', '179728', '179736', '179745']
    nosy_count = 5.0
    nosy_names = ['doerwalter', 'rhettinger', 'vinay.sajip', 'r.david.murray', 'python-dev']
    pr_nums = []
    priority = 'normal'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'enhancement'
    url = 'https://bugs.python.org/issue16613'
    versions = ['Python 3.4']

    @vsajip
    Copy link
    Member Author

    vsajip commented Dec 5, 2012

    ChainMap.new_child could IMO be improved through allowing an optional dict to be passed, which is used to create the child. The use case is that you sometimes need to temporarily push a new non-empty mapping in front of an existing chain. This could be achieved by changing new_child to the following, which is backwards-compatible:

        def new_child(self, d=None):
            'New ChainMap with a new dict followed by all previous maps.'
            return self.__class__(d or {}, *self.maps)

    @vsajip vsajip added stdlib Python modules in the Lib dir type-feature A feature request or enhancement labels Dec 5, 2012
    @bitdancer
    Copy link
    Member

    I agree that this would be useful.

    @doerwalter
    Copy link
    Contributor

    I'd like to have this feature too. However the code should use

    d if d is not None else {}

    instead of

    d or {}

    For example I might want to use a subclass of dict (lowerdict) that converts all keys to lowercase. When I use an empty lowerdict in new_child(), new_child() would silently use a normal dict instead:

       class lowerdict(dict):
           def __getitem__(self, key):
               return dict.__getitem__(
                   self,
                   key.lower() if isinstance(key, str) else key
               )
       
       import collections
       
       cm = collections.ChainMap(lowerdict(), lowerdict())
       
       cm2 = cm.new_child(lowerdict())
       
       print(type(cm2.maps[0]))

    This would print <class 'dict'>.

    @vsajip
    Copy link
    Member Author

    vsajip commented Jan 10, 2013

    d if d is not None else {}

    Your intention makes sense, though I would prefer to write it as:

        if d is None:
            d = {}
        return self.__class__(d, *self.maps)

    @rhettinger rhettinger self-assigned this Jan 11, 2013
    @rhettinger
    Copy link
    Contributor

    Can you write-up a patch with tests and a doc change?

    @vsajip
    Copy link
    Member Author

    vsajip commented Jan 11, 2013

    Can you write-up a patch with tests and a doc change?

    Done.

    @rhettinger
    Copy link
    Contributor

    Put a *versionchanged* tag in the doc entry and this is ready to apply.

    @rhettinger rhettinger assigned vsajip and unassigned rhettinger Jan 11, 2013
    @rhettinger
    Copy link
    Contributor

    Also, please change the variable name from *amap* to either *m* or *mapping*.

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Jan 11, 2013

    New changeset c0ddae67f4df by Vinay Sajip in branch 'default':
    Closes bpo-16613: Added optional mapping argument to ChainMap.new_child.
    http://hg.python.org/cpython/rev/c0ddae67f4df

    @python-dev python-dev mannequin closed this as completed Jan 11, 2013
    @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
    stdlib Python modules in the Lib dir type-feature A feature request or enhancement
    Projects
    None yet
    Development

    No branches or pull requests

    4 participants