This issue tracker has been migrated to GitHub, and is currently read-only.
For more information, see the GitHub FAQs in the Python's Developer Guide.

classification
Title: Clarify str.isidentifier docstring; fix keyword.iskeyword docstring
Type: behavior Stage: resolved
Components: Documentation Versions: Python 3.8, Python 3.7, Python 3.6, Python 2.7
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: docs@python Nosy List: CuriousLearner, cheryl.sabella, dabeaz, docs@python, r.david.murray, rhettinger, serhiy.storchaka, terry.reedy, willingc
Priority: normal Keywords: easy, patch

Created on 2018-03-06 15:50 by dabeaz, last changed 2022-04-11 14:58 by admin. This issue is now closed.

Pull Requests
URL Status Linked Edit
PR 6088 merged CuriousLearner, 2018-03-12 17:47
PR 9756 merged lelit, 2018-10-08 10:27
Messages (22)
msg313335 - (view) Author: David Beazley (dabeaz) Date: 2018-03-06 15:50
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 ;-).
msg313338 - (view) Author: Sanyam Khurana (CuriousLearner) * (Python triager) Date: 2018-03-06 16:06
Hey David,

I'm working on this issue. Will submit a PR in a while :)
msg313499 - (view) Author: Terry J. Reedy (terry.reedy) * (Python committer) Date: 2018-03-09 20:26
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'.
msg313504 - (view) Author: David Beazley (dabeaz) Date: 2018-03-09 20:44
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 ;-)
msg313533 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2018-03-10 15:16
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.
msg313540 - (view) Author: Terry J. Reedy (terry.reedy) * (Python committer) Date: 2018-03-10 16:24
I don't think my suggestion is much better either, and I otherwise agree with Serhiy.  There are a thousand+ more important doc issues.
msg313542 - (view) Author: David Beazley (dabeaz) Date: 2018-03-10 16:27
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.
msg313544 - (view) Author: Terry J. Reedy (terry.reedy) * (Python committer) Date: 2018-03-10 16:53
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."
msg313548 - (view) Author: Carol Willing (willingc) * (Python committer) Date: 2018-03-10 17:11
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.
msg313550 - (view) Author: Terry J. Reedy (terry.reedy) * (Python committer) Date: 2018-03-10 17:44
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.
msg313673 - (view) Author: Sanyam Khurana (CuriousLearner) * (Python triager) Date: 2018-03-12 17:49
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?
msg313676 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2018-03-12 18:16
The change that will add a docstring to keyword.iskeyword() inevitable will have a negative performance effect. Is it worth it?
msg313682 - (view) Author: Terry J. Reedy (terry.reedy) * (Python committer) Date: 2018-03-12 19:30
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?
msg313688 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2018-03-12 20:01
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.
msg313694 - (view) Author: Carol Willing (willingc) * (Python committer) Date: 2018-03-12 20:25
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.
msg313697 - (view) Author: Cheryl Sabella (cheryl.sabella) * (Python committer) Date: 2018-03-12 20:38
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
msg313712 - (view) Author: Terry J. Reedy (terry.reedy) * (Python committer) Date: 2018-03-12 23:30
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.
msg314019 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2018-03-18 03:32
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".
msg314507 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2018-03-27 09:38
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.
msg314525 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2018-03-27 13:41
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.
msg327325 - (view) Author: Carol Willing (willingc) * (Python committer) Date: 2018-10-08 06:53
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/ffc5a14d00db984c8e72c7b67da8a493e17e2c14
msg327356 - (view) Author: Sanyam Khurana (CuriousLearner) * (Python triager) Date: 2018-10-08 15:28
Marking this fixed via https://github.com/python/cpython/commit/ffc5a14d00db984c8e72c7b67da8a493e17e2c14 and https://github.com/python/cpython/commit/fc8205cb4b87edd1c19e1bcc26deaa1570f87988

Thanks, everyone!
History
Date User Action Args
2022-04-11 14:58:58adminsetgithub: 77195
2018-10-08 15:28:17CuriousLearnersetstatus: open -> closed
resolution: fixed
messages: + msg327356

stage: patch review -> resolved
2018-10-08 10:27:07lelitsetkeywords: + patch
stage: needs patch -> patch review
pull_requests: + pull_request9141
2018-10-08 06:53:37willingcsetmessages: + msg327325
2018-03-27 13:41:30r.david.murraysetmessages: + msg314525
2018-03-27 09:38:18serhiy.storchakasetmessages: + msg314507
2018-03-18 03:32:22r.david.murraysetnosy: + r.david.murray
messages: + msg314019
2018-03-12 23:30:13terry.reedysetmessages: + msg313712
2018-03-12 20:38:13cheryl.sabellasetnosy: + cheryl.sabella
messages: + msg313697
2018-03-12 20:25:02willingcsetmessages: + msg313694
2018-03-12 20:01:50serhiy.storchakasetmessages: + msg313688
2018-03-12 19:30:02terry.reedysetmessages: + msg313682
2018-03-12 18:16:25serhiy.storchakasetnosy: + rhettinger, serhiy.storchaka
messages: + msg313676
2018-03-12 17:49:14CuriousLearnersetkeywords: - patch

messages: + msg313673
stage: patch review -> needs patch
2018-03-12 17:47:25CuriousLearnersetkeywords: + patch
stage: needs patch -> patch review
pull_requests: + pull_request5850
2018-03-10 17:51:44terry.reedysetversions: + Python 2.7, Python 3.6, Python 3.8
2018-03-10 17:51:08terry.reedysetnosy: + 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
2018-03-10 17:44:28terry.reedysetnosy: - willingc

messages: + msg313550
title: Clarify doc string for str.isidentifier() -> Clarify str.isidentifier docstring, fix keyword.iskeyword docstring
2018-03-10 17:11:50willingcsetnosy: + willingc

messages: + msg313548
title: Clarify str.isidentifier docstring, fix keyword.iskeyword docstring -> Clarify doc string for str.isidentifier()
2018-03-10 16:55:10terry.reedysettitle: Clarify doc string for str.isidentifier() -> Clarify str.isidentifier docstring, fix keyword.iskeyword docstring
2018-03-10 16:53:39terry.reedysetstatus: closed -> open
versions: + Python 2.7, Python 3.6, Python 3.8
type: behavior
messages: + msg313544

resolution: rejected -> (no value)
stage: resolved -> needs patch
2018-03-10 16:27:32dabeazsetmessages: + msg313542
2018-03-10 16:24:47terry.reedysetstatus: open -> closed
resolution: rejected
messages: + msg313540

stage: resolved
2018-03-10 15:16:36serhiy.storchakasetnosy: + serhiy.storchaka
messages: + msg313533
2018-03-09 20:44:25dabeazsetmessages: + msg313504
2018-03-09 20:26:06terry.reedysetnosy: + terry.reedy
messages: + msg313499
2018-03-07 08:00:51rhettingersetkeywords: + easy
2018-03-06 16:06:23CuriousLearnersetnosy: + CuriousLearner
messages: + msg313338
2018-03-06 15:50:40dabeazcreate