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

Argument Clinic: convert dict methods #73497

Closed
vstinner opened this issue Jan 18, 2017 · 19 comments
Closed

Argument Clinic: convert dict methods #73497

vstinner opened this issue Jan 18, 2017 · 19 comments
Labels
3.7 (EOL) end of life performance Performance or resource usage topic-argument-clinic

Comments

@vstinner
Copy link
Member

BPO 29311
Nosy @rhettinger, @vstinner, @larryhastings, @methane, @ericsnowcurrently, @vadmium, @serhiy-storchaka
Files
  • dict.patch
  • dict-docstrings.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 = None
    closed_at = <Date 2017-02-01.16:11:52.450>
    created_at = <Date 2017-01-18.16:54:10.235>
    labels = ['3.7', 'expert-argument-clinic', 'performance']
    title = 'Argument Clinic: convert dict methods'
    updated_at = <Date 2017-05-03.16:41:25.389>
    user = 'https://github.com/vstinner'

    bugs.python.org fields:

    activity = <Date 2017-05-03.16:41:25.389>
    actor = 'vstinner'
    assignee = 'none'
    closed = True
    closed_date = <Date 2017-02-01.16:11:52.450>
    closer = 'vstinner'
    components = ['Argument Clinic']
    creation = <Date 2017-01-18.16:54:10.235>
    creator = 'vstinner'
    dependencies = []
    files = ['46331', '46374']
    hgrepos = []
    issue_num = 29311
    keywords = ['patch']
    message_count = 19.0
    messages = ['285742', '285746', '285765', '285775', '285812', '285813', '285818', '285819', '285821', '285845', '285968', '285981', '286221', '286222', '286644', '286912', '286918', '292910', '292922']
    nosy_count = 8.0
    nosy_names = ['rhettinger', 'vstinner', 'larry', 'methane', 'python-dev', 'eric.snow', 'martin.panter', 'serhiy.storchaka']
    pr_nums = []
    priority = 'normal'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'performance'
    url = 'https://bugs.python.org/issue29311'
    versions = ['Python 3.7']

    @vstinner
    Copy link
    Member Author

    The dict type has multiple methods which use METH_VARARGS or METH_VARARGS|METH_KEYWORDS calling convention, whereras there is no a new faster METH_FASTCALL calling convention which avoids temporary tuple/dict to pass arguments.

    @vstinner vstinner added 3.7 (EOL) end of life topic-argument-clinic performance Performance or resource usage labels Jan 18, 2017
    @vstinner
    Copy link
    Member Author

    dict.patch converts two methods to Argument Clinic:

    • get()
    • setdefault()

    pop() requires the issue bpo-29299, its signature must not show any default value for the second parameter.

    dict.update() is special, so I created a dedicated issue: issue bpo-29312.

    @methane
    Copy link
    Member

    methane commented Jan 19, 2017

    LGTM

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Jan 19, 2017

    New changeset 00c63ee66e0c by Victor Stinner in branch 'default':
    dict.get() and dict.setdefault() now use AC
    https://hg.python.org/cpython/rev/00c63ee66e0c

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Jan 19, 2017

    New changeset fe1d83fe29d6 by Serhiy Storchaka in branch 'default':
    Issue bpo-29311: Argument Clinic generates reasonable name for the parameter "default".
    https://hg.python.org/cpython/rev/fe1d83fe29d6

    @serhiy-storchaka
    Copy link
    Member

    There are same problems with docstrings as in OrderedDict. The name "D" is not defined.

    @vstinner
    Copy link
    Member Author

    Argument Clinic generates reasonable name for the parameter "default".

    • default as failobj: object = None
    • default: object = None

    Thanks, that's a better name :-)

    FYI I wanted to limit changes when converting to AC, the change is already big enough and hard to review.

    @vstinner
    Copy link
    Member Author

    There are same problems with docstrings as in OrderedDict. The name "D" is not defined.

    I copied the old docstring to AC.

    Python 3.6 doc:
    ---

    get(...)
        D.get(k[,d]) -> D[k] if k in D, else d.  d defaults to None.

    D was already implicitly "self".

    Python 3.7 (new) doc:
    ---

    get(self, key, default=None, /)
        D.get(key[, default]) -> D[key] if key in D, else default.

    What do you propose? Use self?

    @serhiy-storchaka
    Copy link
    Member

    There was a context in old docstrings removed in new docstrings.

    It is not always possible just to copy docstrings in Argument Clinic. Sometimes rewriting docstrings is the hardest part of converting to Argument Clinic.

    I suggest either restore the context that defines D, or better totally rewrite docstrings, getting the wording from the documentation.

    @vadmium
    Copy link
    Member

    vadmium commented Jan 19, 2017

    D.get(key[, default]) -> D[key] if key in D, else default.

    There is no big problem with that. D is defined at the start. The only thing I would have suggested is avoid using square brackets to mean two things in the one expression. Since it is no longer the signature, calling with both parameters should be okay:

    '''
    get($self, key, default=None, /)

    D.get(key, default) -> D[key] if key in D, else default.
    '''

    However the other method no longer defines D:

    '''
    setdefault($self, key, default=None, /)

    D.get(key,default), also set D[key]=default if key not in D.
    '''

    You could restore the initial text as “D.setdefault(key,default) ->”, or maybe rewrite it like

    “Like get(), but also insert the default value if it is missing.”

    @serhiy-storchaka
    Copy link
    Member

    Proposed patch fixes/improves docstrings of dict and OrderedDict methods.

    @vadmium
    Copy link
    Member

    vadmium commented Jan 22, 2017

    Patch looks good, apart from one little thing (see review)

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Jan 24, 2017

    New changeset ffc0840762e4 by Serhiy Storchaka in branch 'default':
    Issues bpo-29311, bpo-29289: Fixed and improved docstrings for dict and OrderedDict
    https://hg.python.org/cpython/rev/ffc0840762e4

    @serhiy-storchaka
    Copy link
    Member

    Thank you for your review Martin.

    @vstinner
    Copy link
    Member Author

    vstinner commented Feb 1, 2017

    Thank you very much for the docstring enhancement Serhiy! I like your new docstrings.

    It seems like all known issues are now fixed, so I close the issue. Thanks Naoki and Martin too for the reviews.

    @vstinner vstinner closed this as completed Feb 1, 2017
    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Feb 4, 2017

    New changeset 222b9392a83b by Serhiy Storchaka in branch 'default':
    Issue bpo-29311: Regenerate Argument Clinic.
    https://hg.python.org/cpython/rev/222b9392a83b

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Feb 4, 2017

    New changeset 7baf0a0b90da5bb0b1ed5d27374f5a6b1c7b5dae by Serhiy Storchaka in branch 'master':
    Issue bpo-29311: Regenerate Argument Clinic.
    7baf0a0

    @rhettinger
    Copy link
    Contributor

    IMO, you've made the docstrings for pure Python versions of collections.OrderedDict worse. The previous docstrings were clearer.

    Please note, there is a high bar for altering docstrings that have worked well (been deployed with success) for over 15 years (they were modeled on the dict object docstrings). You're undoing some of the work originally done by Guido van Rossum and Tim Peters.

    Argument clinic was accepted on the premise that it would add useful information, not that it would entail a wholesale rewrite of every docstring.

    @vstinner
    Copy link
    Member Author

    vstinner commented May 3, 2017

    IMO, you've made the docstrings for pure Python versions of collections.OrderedDict worse. The previous docstrings were clearer.

    Can you please elaborate? Please reopen the issue or open a new one if you consider that it's worth it (this issue is currently closed).

    @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 (EOL) end of life performance Performance or resource usage topic-argument-clinic
    Projects
    None yet
    Development

    No branches or pull requests

    5 participants