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

Constructors of some mapping classes don't accept self keyword argument #66799

Closed
abacabadabacaba mannequin opened this issue Oct 11, 2014 · 34 comments
Closed

Constructors of some mapping classes don't accept self keyword argument #66799

abacabadabacaba mannequin opened this issue Oct 11, 2014 · 34 comments
Assignees
Labels
stdlib Python modules in the Lib dir type-bug An unexpected behavior, bug, or error

Comments

@abacabadabacaba
Copy link
Mannequin

abacabadabacaba mannequin commented Oct 11, 2014

BPO 22609
Nosy @rhettinger, @mdickinson, @vstinner, @larryhastings, @berkerpeksag, @vadmium, @serhiy-storchaka, @1st1
Files
  • collections_pos_only_params.patch
  • collections_pos_only_params2.patch
  • UserDict_self_and_dict_keywords.patch
  • UserDict_self_and_dict_keywords_2.patch
  • UserDict_self_and_dict_keywords_3.patch
  • 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/rhettinger'
    closed_at = <Date 2015-09-29.20:59:53.927>
    created_at = <Date 2014-10-11.12:04:09.064>
    labels = ['type-bug', 'library']
    title = "Constructors of some mapping classes don't accept `self` keyword argument"
    updated_at = <Date 2015-09-29.20:59:53.926>
    user = 'https://bugs.python.org/abacabadabacaba'

    bugs.python.org fields:

    activity = <Date 2015-09-29.20:59:53.926>
    actor = 'serhiy.storchaka'
    assignee = 'rhettinger'
    closed = True
    closed_date = <Date 2015-09-29.20:59:53.927>
    closer = 'serhiy.storchaka'
    components = ['Library (Lib)']
    creation = <Date 2014-10-11.12:04:09.064>
    creator = 'abacabadabacaba'
    dependencies = []
    files = ['36881', '36886', '38692', '38754', '39821']
    hgrepos = []
    issue_num = 22609
    keywords = ['patch']
    message_count = 34.0
    messages = ['229076', '229089', '229098', '229118', '229121', '229189', '229248', '229249', '231753', '231763', '231765', '231766', '231769', '231915', '231936', '232353', '234875', '239278', '239655', '239667', '243329', '244470', '244503', '245859', '245877', '245944', '246936', '246954', '246956', '246957', '251778', '251831', '251886', '251888']
    nosy_count = 10.0
    nosy_names = ['rhettinger', 'mark.dickinson', 'vstinner', 'larry', 'abacabadabacaba', 'python-dev', 'berker.peksag', 'martin.panter', 'serhiy.storchaka', 'yselivanov']
    pr_nums = []
    priority = 'normal'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'behavior'
    url = 'https://bugs.python.org/issue22609'
    versions = ['Python 2.7', 'Python 3.4', 'Python 3.5', 'Python 3.6']

    @abacabadabacaba
    Copy link
    Mannequin Author

    abacabadabacaba mannequin commented Oct 11, 2014

    >>> import collections
    >>> collections.Counter(self=1)
    Traceback (most recent call last):
      File "<stdin>", line 1, in <module>
    TypeError: __init__() got multiple values for argument 'self'
    >>> collections.OrderedDict(self="test")
    Traceback (most recent call last):
      File "<stdin>", line 1, in <module>
    TypeError: __init__() got multiple values for argument 'self'
    >>> collections.UserDict(self="test")
    Traceback (most recent call last):
      File "<stdin>", line 1, in <module>
    TypeError: __init__() got multiple values for argument 'self'

    Python surely should have positional-only parameters.

    @abacabadabacaba abacabadabacaba mannequin added stdlib Python modules in the Lib dir type-bug An unexpected behavior, bug, or error labels Oct 11, 2014
    @serhiy-storchaka
    Copy link
    Member

    Here is a patch.

    @rhettinger rhettinger self-assigned this Oct 11, 2014
    @rhettinger
    Copy link
    Contributor

    I'll spend some time taking this one under consideration.

    Keyword arguments for dicts and dict-like classes are already somewhat limited (only non-keyword identifiers) that why Guido resisted adding them in the first place. And, in the context of OrderedDicts, they aren't really useful at all (because the order gets scrambled).

    I'm reluctant to make the number of changes required in order to support this corner case. The ship for Python 2.7 sailed a long time ago and it is risky to make changes like this. After the patch, the code is less readable, harder to get right, harder to maintain, and not as performant.

    On the other hand, adding "self" would be a nice-to-have, so it is worth thinking about.

    @rhettinger
    Copy link
    Contributor

    After some thought, I think we have to fix this.

    I'll go through the patch in careful detail this weekend.

    Ethan, if you would like to help, it would be great to have a third pair of eyes looking at the patch to make sure it correct it every detail.

    @serhiy-storchaka
    Copy link
    Member

    I thought that more verbose but straightforward code would be more acceptable.
    Well, here is smaller and clever patch. Tests are the same.

    @ethanfurman
    Copy link
    Member

    I will take a look.

    @ethanfurman
    Copy link
    Member

    Code looks good.

    Only downside is the change in help and inspect.signature output, but that is minor:

    Help on dict object:
    class dict(object)
    [...]
    | __init__(self, /, *args, **kwargs)

    vs.

    Help on class Counter in module collections:
    class Counter(builtins.dict)
    [...]
    | __init__(*args, **kwds)

    +1 to accept.

    @larryhastings
    Copy link
    Contributor

    FWIW, I agree that it should be fixed:

    >>> dict(self=1)
    {'self': 1}

    @rhettinger
    Copy link
    Contributor

    This looks good. Go ahead and apply the first version of the patch.

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Nov 27, 2014

    New changeset 816c15fe5812 by Serhiy Storchaka in branch '3.4':
    Issue bpo-22609: Constructors and update methods of mapping classes in the
    https://hg.python.org/cpython/rev/816c15fe5812

    New changeset 88ab046fdd8a by Serhiy Storchaka in branch 'default':
    Issue bpo-22609: Constructors and update methods of mapping classes in the
    https://hg.python.org/cpython/rev/88ab046fdd8a

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Nov 27, 2014

    New changeset cd1ead4feddf by Serhiy Storchaka in branch '3.4':
    Issue bpo-22609: Revert changes in UserDict. They conflicted with existing tests.
    https://hg.python.org/cpython/rev/cd1ead4feddf

    New changeset 167d51a54de2 by Serhiy Storchaka in branch 'default':
    Issue bpo-22609: Revert changes in UserDict. They conflicted with existing tests.
    https://hg.python.org/cpython/rev/167d51a54de2

    @serhiy-storchaka
    Copy link
    Member

    Unfortunately there is existing test for current behavior of UserDict:

    self.assertEqual(collections.UserDict(dict=[('one',1), ('two',2)]), d2)
    

    This looks wrong to me and I think we should break compatibility here.

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Nov 27, 2014

    New changeset 3dfe4f0c626b by Serhiy Storchaka in branch '2.7':
    Issue bpo-22609: Constructors and update methods of mapping classes in the
    https://hg.python.org/cpython/rev/3dfe4f0c626b

    @serhiy-storchaka
    Copy link
    Member

    So what to do wish UserDict? Should we break backward compatibility and add support for "self" and "dict" keywords as in other dict-like types?

    @ethanfurman
    Copy link
    Member

    Fix self now, add a warning and single minor cycle deprecation period for 'dict'.

    @rhettinger
    Copy link
    Contributor

    [Serhiy]

    So what to do wish UserDict?

    I'm leaning in favor of leaving UserDict as-is. AFAICT, in the very long history of UserDict, this has never been a problem. So, I don't think there is an issue worth breaking the published API and possibly breaking code that currently works. (This is a case of practicality beating purity).

    @serhiy-storchaka
    Copy link
    Member

    So may be close this issue?

    See also bpo-22958.

    @serhiy-storchaka
    Copy link
    Member

    Here is a patch for UserDict, that keep current behavior, but allows to pass keys "self" and "dict" if positional parameter dict is specified.

    >>> UserDict(self=42)
    {'self': 42}
    >>> UserDict({}, dict=42)
    {'dict': 42}
    >>> UserDict(dict={'a': 42})
    {'a': 42}

    @berkerpeksag
    Copy link
    Member

    UserDict_self_and_dict_keywords.patch looks good to me. Is there a reason not to use assertWarnsRegex?

    Also, there are already collections.UserDict() usages in the test file, so I'd remove the "from collections import UserDict" import.

    @serhiy-storchaka
    Copy link
    Member

    Thank you for your review Berker.

    Is there a reason not to use assertWarnsRegex?

    Initially the patch was written for 2.7. Fixing WeakValueDictionary in 2.7
    needs first fix UserDict (bpo-22958). That is why I returned to this issue.

    Also, there are already collections.UserDict() usages in the test file, so
    I'd remove the "from collections import UserDict" import.

    These tests originally was written for test_collections.

    Updated patch addresses Berker's comment.

    @serhiy-storchaka
    Copy link
    Member

    Could you please look at the new patch Raymond? This is the dependency for bpo-22958.

    @serhiy-storchaka
    Copy link
    Member

    Ping.

    @vadmium
    Copy link
    Member

    vadmium commented May 31, 2015

    Left a couple pedantic comments.

    @serhiy-storchaka
    Copy link
    Member

    Updated patch addresses Martin's comments.

    @1st1
    Copy link
    Member

    1st1 commented Jun 27, 2015

    Left some feedback in the code review.

    @vadmium
    Copy link
    Member

    vadmium commented Jun 29, 2015

    Patch looks fine to me. I understand Yury withdrew his comment.

    @serhiy-storchaka
    Copy link
    Member

    Ping again.

    @vadmium
    Copy link
    Member

    vadmium commented Jul 20, 2015

    Who are you pinging? I did just notice a minor English grammar problem (“one arguments”). But as far as I am concered you could have already committed the patch.

    @rhettinger
    Copy link
    Contributor

    I will look at this more when I get a chance (likely this week).

    @serhiy-storchaka
    Copy link
    Member

    I was pinging Raymond. He is maintainer of the collections module, this issue is assigned to his, and he had valid objections to previous version of the patch. Even one of this reason is enough to wait his review before committing.

    Thank you Raymond.

    @serhiy-storchaka
    Copy link
    Member

    Ping.

    @vstinner
    Copy link
    Member

    UserDict_self_and_dict_keywords_3.patch looks good to me.

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Sep 29, 2015

    New changeset 4c5407e1b0ec by Serhiy Storchaka in branch '2.7':
    Issue bpo-22609: Constructor and the update method of collections.UserDict now
    https://hg.python.org/cpython/rev/4c5407e1b0ec

    New changeset 1869f5625392 by Serhiy Storchaka in branch '3.4':
    Issue bpo-22609: Constructor of collections.UserDict now accepts the self keyword
    https://hg.python.org/cpython/rev/1869f5625392

    New changeset ab7e3f1f9f88 by Serhiy Storchaka in branch '3.5':
    Issue bpo-22609: Constructor of collections.UserDict now accepts the self keyword
    https://hg.python.org/cpython/rev/ab7e3f1f9f88

    New changeset 901964295066 by Serhiy Storchaka in branch 'default':
    Issue bpo-22609: Constructor of collections.UserDict now accepts the self keyword
    https://hg.python.org/cpython/rev/901964295066

    @serhiy-storchaka
    Copy link
    Member

    Thank you all for your reviews.

    @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-bug An unexpected behavior, bug, or error
    Projects
    None yet
    Development

    No branches or pull requests

    8 participants