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
Comments
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. |
LGTM |
New changeset 00c63ee66e0c by Victor Stinner in branch 'default': |
New changeset fe1d83fe29d6 by Serhiy Storchaka in branch 'default': |
There are same problems with docstrings as in OrderedDict. The name "D" is not defined. |
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. |
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? |
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. |
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: '''
|
Proposed patch fixes/improves docstrings of dict and OrderedDict methods. |
Patch looks good, apart from one little thing (see review) |
New changeset ffc0840762e4 by Serhiy Storchaka in branch 'default': |
Thank you for your review Martin. |
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. |
New changeset 222b9392a83b by Serhiy Storchaka in branch 'default': |
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. |
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). |
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:
bugs.python.org fields:
The text was updated successfully, but these errors were encountered: