[issue33014] Clarify doc string for str.isidentifier()
New submission from David Beazley <dave@dabeaz.com>: This is a minor nit, but the doc string for str.isidentifier() states: Use keyword.iskeyword() to test for reserved identifiers such as "def" and "class". At first glance, I thought that it meant you'd do this (doesn't work): 'def'.iskeyword() As opposed to this: import keyword keyword.iskeyword('def') Perhaps a clarification that "keyword" refers to the keyword module could be added. Or better yet, just make 'iskeyword()` a string method ;-). ---------- assignee: docs@python components: Documentation messages: 313335 nosy: dabeaz, docs@python priority: normal severity: normal status: open title: Clarify doc string for str.isidentifier() versions: Python 3.7 _______________________________________ Python tracker <report@bugs.python.org> <https://bugs.python.org/issue33014> _______________________________________
Sanyam Khurana <sanyam.khurana01@gmail.com> added the comment: Hey David, I'm working on this issue. Will submit a PR in a while :) ---------- nosy: +CuriousLearner _______________________________________ Python tracker <report@bugs.python.org> <https://bugs.python.org/issue33014> _______________________________________
Change by Raymond Hettinger <raymond.hettinger@gmail.com>: ---------- keywords: +easy _______________________________________ Python tracker <report@bugs.python.org> <https://bugs.python.org/issue33014> _______________________________________
Terry J. Reedy <tjreedy@udel.edu> added the comment: No new string method ;-). "Call function keyword.iskeyword() ..." should make it clear that iskeyword is not a string method. Samyam, I suggest getting agreement on new wording first. Then do PR that is more or less 'pre-approved'. ---------- nosy: +terry.reedy _______________________________________ Python tracker <report@bugs.python.org> <https://bugs.python.org/issue33014> _______________________________________
David Beazley <dave@dabeaz.com> added the comment: That wording isn't much better in my opinion. If I'm sitting there looking at methods like str.isdigit(), str.isnumeric(), str.isascii(), and str.isidentifier(), seeing keyword.iskeyword() makes me think it's a method regardless of whether you label it a function or method. Explicitly stating that "keyword" is actually the keyword module makes it much clearer. Or at least including the argument as well keyword.iskeyword(kw). It really should be a string method though ;-) ---------- _______________________________________ Python tracker <report@bugs.python.org> <https://bugs.python.org/issue33014> _______________________________________
Serhiy Storchaka <storchaka+cpython@gmail.com> added the comment: If this docstring needs an improvement Terry's wording LGTM. I'm -1 for further complicating it. Where did you get the idea that iskeyword() is a string method? Nothing in the docstring points to this. "keyword" is not used as an alias of "str". It is not used anywhere else in the docstring at all. I think this is an attempt to solve a non-problem. ---------- nosy: +serhiy.storchaka _______________________________________ Python tracker <report@bugs.python.org> <https://bugs.python.org/issue33014> _______________________________________
Terry J. Reedy <tjreedy@udel.edu> added the comment: I don't think my suggestion is much better either, and I otherwise agree with Serhiy. There are a thousand+ more important doc issues. ---------- resolution: -> rejected stage: -> resolved status: open -> closed _______________________________________ Python tracker <report@bugs.python.org> <https://bugs.python.org/issue33014> _______________________________________
David Beazley <dave@dabeaz.com> added the comment: s = 'Some String' s.isalnum() s.isalpha() s.isdecimal() s.isdigit() s.isidentifier() s.islower() s.isnumeric() s.isprintable() s.isspace() s.istitle() s.isupper() Not really sure where I would have gotten the idea that it might be referring to s.iskeyword(). But what do I know? I'll stop submitting further suggestions. ---------- _______________________________________ Python tracker <report@bugs.python.org> <https://bugs.python.org/issue33014> _______________________________________
Terry J. Reedy <tjreedy@udel.edu> added the comment: Part of my reason for closing is that we have an index entry "iskeyword() (in module keyword)" to help anyone who tries the function as a method. But I am reopening for 2 reasons: 1. I think the following is a better improvement that reads better as well as being more informative. I would be willing to merge it. "Call keyword.iskeyword(s) to test whether string s is a reserved identifier, such as "def" or "class". 2. We should fix the buggy iskeyword docstring in 2.7 and 3.x.
keyword.iskeyword.__doc__ 'x.__contains__(y) <==> y in x.'
Replace with the doc entry: "Return true if s is a Python keyword." ---------- resolution: rejected -> stage: resolved -> needs patch status: closed -> open type: -> behavior versions: +Python 2.7, Python 3.6, Python 3.8 _______________________________________ Python tracker <report@bugs.python.org> <https://bugs.python.org/issue33014> _______________________________________
Change by Terry J. Reedy <tjreedy@udel.edu>: ---------- title: Clarify doc string for str.isidentifier() -> Clarify str.isidentifier docstring, fix keyword.iskeyword docstring _______________________________________ Python tracker <report@bugs.python.org> <https://bugs.python.org/issue33014> _______________________________________
Carol Willing <willingc@gmail.com> added the comment: Terry's latest documentation suggestion sounds good and "explicit" by including an example. David, I appreciate your doc suggestions. If it's befuddling you, it's likely doing the same for others. CuriousLearner, do you wish to try to incorporate Terry's latest suggestions? Thanks all. ---------- nosy: +willingc title: Clarify str.isidentifier docstring, fix keyword.iskeyword docstring -> Clarify doc string for str.isidentifier() _______________________________________ Python tracker <report@bugs.python.org> <https://bugs.python.org/issue33014> _______________________________________
Terry J. Reedy <tjreedy@udel.edu> added the comment: Debating historical design decisions that effectively cannot be changed is off topic for the tracker (but fair game on python-list). However, I will explain this much. The s.isxyz questions are answered by examining the characters and codepoints in s. Answering iskeyword(s) requires reference to the collection of keywords, kwlist, which must be exposed to users, and which may change in any new version. We usually attach constant data attributes to modules (other than builtins), and never (that I can think of) to built-in classes. "def iskeyword(s): return s in kwlist:" belongs in the module with kwlist. ---------- nosy: -willingc title: Clarify doc string for str.isidentifier() -> Clarify str.isidentifier docstring, fix keyword.iskeyword docstring _______________________________________ Python tracker <report@bugs.python.org> <https://bugs.python.org/issue33014> _______________________________________
Change by Terry J. Reedy <tjreedy@udel.edu>: ---------- nosy: +willingc -serhiy.storchaka title: Clarify str.isidentifier docstring, fix keyword.iskeyword docstring -> Clarify str.isidentifier docstring; fix keyword.iskeyword docstring versions: -Python 2.7, Python 3.6, Python 3.8 _______________________________________ Python tracker <report@bugs.python.org> <https://bugs.python.org/issue33014> _______________________________________
Change by Terry J. Reedy <tjreedy@udel.edu>: ---------- versions: +Python 2.7, Python 3.6, Python 3.8 _______________________________________ Python tracker <report@bugs.python.org> <https://bugs.python.org/issue33014> _______________________________________
Change by Sanyam Khurana <sanyam.khurana01@gmail.com>: ---------- keywords: +patch pull_requests: +5850 stage: needs patch -> patch review _______________________________________ Python tracker <report@bugs.python.org> <https://bugs.python.org/issue33014> _______________________________________
Sanyam Khurana <sanyam.khurana01@gmail.com> added the comment: Carol, Yes, I've raised a PR. Currently, I've updated the docs for `str.isidentifier` clarifying the usage of `keyword.iskeyword` For updating the docstring of `keyword.iskeyword`, I saw that `Lib/Keyword.py` defines this on line 55: `iskeyword = frozenset(kwlist).__contains__` The docstring of the file says that it is automatically generated from `graminit.c`. I observed that file and have no clue on how to proceed to have the doc string updated. Can someone provide me a pointer on this please? ---------- keywords: -patch stage: patch review -> needs patch _______________________________________ Python tracker <report@bugs.python.org> <https://bugs.python.org/issue33014> _______________________________________
Serhiy Storchaka <storchaka+cpython@gmail.com> added the comment: The change that will add a docstring to keyword.iskeyword() inevitable will have a negative performance effect. Is it worth it? ---------- nosy: +rhettinger, serhiy.storchaka _______________________________________ Python tracker <report@bugs.python.org> <https://bugs.python.org/issue33014> _______________________________________
Terry J. Reedy <tjreedy@udel.edu> added the comment: As I posted above, keywork.iskeyword already has a bizarrely incorrect docstring, so how can there be a performance impact? And why single out such a rarely used function? ---------- _______________________________________ Python tracker <report@bugs.python.org> <https://bugs.python.org/issue33014> _______________________________________
Serhiy Storchaka <storchaka+cpython@gmail.com> added the comment: For changing a docstring we have to change the implementation of iskeyword(). It seems to me that the current implementation is the fastest, and any other implementation will be a tiny bit slower. I just wanted to say that this enhancement has a non-zero cost. ---------- _______________________________________ Python tracker <report@bugs.python.org> <https://bugs.python.org/issue33014> _______________________________________
Carol Willing <willingc@gmail.com> added the comment: I've made an additional suggestion on the open PR to add an example to the `.rst` doc that better clarifies the differences and usage of `iskeyword` and `isidentifier`. Perhaps making that addition and skipping the updates to the C source code would be a reasonable step forward. While perhaps not ideal, it seems a reasonable compromise to provide more helpful documentation without any potential performance impact or regression. ---------- _______________________________________ Python tracker <report@bugs.python.org> <https://bugs.python.org/issue33014> _______________________________________
Cheryl Sabella <chekat2@gmail.com> added the comment: Too bad we couldn't do: iskeyword = frozenset(kwlist).__contains__ iskeyword.__doc__ = 'Test whether a string is a reserved identifier.' But I'm sure there are reasons for this: AttributeError: attribute '__doc__' of 'builtin_function_or_method' objects is not writable ---------- nosy: +csabella _______________________________________ Python tracker <report@bugs.python.org> <https://bugs.python.org/issue33014> _______________________________________
Terry J. Reedy <tjreedy@udel.edu> added the comment: Separate PRs for doc and code changes will be needed anyway if the code change is restricted to 3.8. To me, changing keyword.iskeyword.__doc__ from the irrelevant Python tautology 'x.__contains__(y) <==> y in x.' to something that says what the function does, as docstrings should, is a bug fix, not an enhancement. Hence a slight slowdown is not a concern to me. I see 4 possible types of fixes. 1. Write a python function with docstring. 3.8 only as it changes type(iskeyword). def iskeyword(s): "Return true if s is a Python keyword." return s in kwlist 2. Change the aberrant set/frozenset.__contains__.__doc__ so it makes some sense as a docstring for a bound method (as with list and dict). list/tuple.__contains__.__doc__ is 'Return key in self.' dict.__contains__.__doc__ is 'True if the dictionary has the specified key, else False.' I would copy the dict __contains__ docstring, with 'Return' prefixed, to set and frozenset, with the obvious substitution. I don't know about backporting this. 3. Make bound_method docstrings writable, like with Python function docstrings (3.8 only). Then we could use Cheryl's suggestion. Or add a function bounddoc(bound_method, docstring) that can change a bound_method's docsting. CPython uses 2 types for built-in methods bound to an instance. The choice is not consistent within a class or across classes for a particular method. builtin_function_or_method ([].append, frozenset().__contains__) method-wrapper ([].__contains__) Python classes result in 'bound method'. All 3 copy the docstring of the instance method. (For Python classes, one can temporarily change the method docstring before creating a new bound method.) 4. Add makebound(method, instance, docstring) that creates a bound method (of the appropriate type) but with the passed docstring (3.8 only) 3 or 4 would allow any public bound method to have a custom docstring. ---------- _______________________________________ Python tracker <report@bugs.python.org> <https://bugs.python.org/issue33014> _______________________________________
R. David Murray <rdmurray@bitdance.com> added the comment: For the original report that this issue was opened for: Use keyword.iskeyword() to test for reserved identifiers such as "def" and "class". The obvious replacement is: Use the iskeyword() function from the keyword module to test for reserved identifiers such as "def" and "class". ---------- nosy: +r.david.murray _______________________________________ Python tracker <report@bugs.python.org> <https://bugs.python.org/issue33014> _______________________________________
Serhiy Storchaka <storchaka+cpython@gmail.com> added the comment: I concur with David about the docstring. But I think that no changes are needed in the module documentation. David's wording would contain two links (for iskeyword() and for keyword), and many links distract the attention. We already spent for this issue more time than it deserves. ---------- _______________________________________ Python tracker <report@bugs.python.org> <https://bugs.python.org/issue33014> _______________________________________
R. David Murray <rdmurray@bitdance.com> added the comment: I think my wording would be an improvement to the docs as well. You could link just the keyword function if you are worried about too many links, since that would keep the link count the same. ---------- _______________________________________ Python tracker <report@bugs.python.org> <https://bugs.python.org/issue33014> _______________________________________
Carol Willing <willingc@gmail.com> added the comment: New changeset ffc5a14d00db984c8e72c7b67da8a493e17e2c14 by Carol Willing (Sanyam Khurana) in branch 'master': bpo-33014: Clarify str.isidentifier docstring (GH-6088) https://github.com/python/cpython/commit/ffc5a14d00db984c8e72c7b67da8a493e17... ---------- _______________________________________ Python tracker <report@bugs.python.org> <https://bugs.python.org/issue33014> _______________________________________
Change by Lele Gaifax <lelegaifax@gmail.com>: ---------- keywords: +patch pull_requests: +9141 stage: needs patch -> patch review _______________________________________ Python tracker <report@bugs.python.org> <https://bugs.python.org/issue33014> _______________________________________
Sanyam Khurana <sanyam.khurana01@gmail.com> added the comment: Marking this fixed via https://github.com/python/cpython/commit/ffc5a14d00db984c8e72c7b67da8a493e17... and https://github.com/python/cpython/commit/fc8205cb4b87edd1c19e1bcc26deaa1570f... Thanks, everyone! ---------- resolution: -> fixed stage: patch review -> resolved status: open -> closed _______________________________________ Python tracker <report@bugs.python.org> <https://bugs.python.org/issue33014> _______________________________________
participants (9)
-
Carol Willing -
Cheryl Sabella
-
David Beazley
-
Lele Gaifax
-
R. David Murray
-
Raymond Hettinger
-
Sanyam Khurana
-
Serhiy Storchaka
-
Terry J. Reedy