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

Always return a list from PyMapping_Keys/PyMapping_Values/PyMapping_Items #72467

Closed
serhiy-storchaka opened this issue Sep 26, 2016 · 5 comments
Closed
Labels
3.7 interpreter-core type-feature

Comments

@serhiy-storchaka
Copy link
Member

@serhiy-storchaka serhiy-storchaka commented Sep 26, 2016

BPO 28280
Nosy @rhettinger, @serhiy-storchaka, @zhangyangyu, @orenmn
PRs
  • #3840
  • 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 2017-10-21.17:18:52.387>
    created_at = <Date 2016-09-26.18:13:46.369>
    labels = ['interpreter-core', 'type-feature', '3.7']
    title = 'Always return a list from PyMapping_Keys/PyMapping_Values/PyMapping_Items'
    updated_at = <Date 2017-10-21.17:18:52.386>
    user = 'https://github.com/serhiy-storchaka'

    bugs.python.org fields:

    activity = <Date 2017-10-21.17:18:52.386>
    actor = 'serhiy.storchaka'
    assignee = 'none'
    closed = True
    closed_date = <Date 2017-10-21.17:18:52.387>
    closer = 'serhiy.storchaka'
    components = ['Interpreter Core']
    creation = <Date 2016-09-26.18:13:46.369>
    creator = 'serhiy.storchaka'
    dependencies = []
    files = []
    hgrepos = []
    issue_num = 28280
    keywords = ['patch']
    message_count = 5.0
    messages = ['277443', '303340', '303348', '303413', '303902']
    nosy_count = 4.0
    nosy_names = ['rhettinger', 'serhiy.storchaka', 'xiang.zhang', 'Oren Milman']
    pr_nums = ['3840']
    priority = 'normal'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'enhancement'
    url = 'https://bugs.python.org/issue28280'
    versions = ['Python 3.7']

    @serhiy-storchaka
    Copy link
    Member Author

    @serhiy-storchaka serhiy-storchaka commented Sep 26, 2016

    PyMapping_Keys(), PyMapping_Values() and PyMapping_Items() can return a list or tuple (because they use PySequence_Fast). But end users of this API often work only with lists and raise an exception if corresponding mapping methods return an instance of other type (actually only for tuples). See Modules/posixmodule.c, Modules/_winapi.c, Modules/_sre.c. The type is even not checked in the latter file.

    Old documentation said that PyMapping_Keys(o) is equivalent to the Python expression list(o.keys()). Maybe it is worth to make this true. Make PyMapping_Keys(o) etc always returning a list. This could simplify the code that uses this C API. Since keys() etc usually return a dict view, a list or a generator, but not a tuple, this unlikely will cause performance regression.

    @serhiy-storchaka serhiy-storchaka added 3.7 interpreter-core type-feature labels Sep 26, 2016
    @orenmn
    Copy link
    Mannequin

    @orenmn orenmn mannequin commented Sep 29, 2017

    I would be happy to write a PR that implements that.

    However, i am not sure which way is better to construct a list from the return
    value (an iterable, hopefully) of keys() etc.:

    • Call PyList_Type() (in each of PyMapping_Keys() etc.) on the iterable, and
      overwrite the error message in case it is a TypeError.
    • Write a helper function iterable_as_list(), which uses PyObject_GetIter() and
      PySequence_List(), and call it in each of PyMapping_Keys() etc..
      (iterable_as_list() would receive "keys" etc., so that it would raise the
      appropriate error message, in case of a TypeError.)

    ISTM that the first one is simpler, but I am not sure about the performance
    difference between them.

    @serhiy-storchaka
    Copy link
    Member Author

    @serhiy-storchaka serhiy-storchaka commented Sep 29, 2017

    PySequence_List() is the simplest way in C. I don't know whether we need special error messages now. But for compatibility keep them.

    @orenmn
    Copy link
    Mannequin

    @orenmn orenmn mannequin commented Sep 30, 2017

    (for knowledge preservation's sake)
    Resolving this issue would also resolve bpo-31486.

    @serhiy-storchaka
    Copy link
    Member Author

    @serhiy-storchaka serhiy-storchaka commented Oct 8, 2017

    New changeset 0ccc0f6 by Serhiy Storchaka (Oren Milman) in branch 'master':
    bpo-28280: Make PyMapping_Keys(), PyMapping_Values() and PyMapping_Items() always return a list (bpo-3840)
    0ccc0f6

    @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
    3.7 interpreter-core type-feature
    Projects
    None yet
    Development

    No branches or pull requests

    1 participant