Clarify str.isidentifier docstring; fix keyword.iskeyword docstring

[dabeaz]

BPO	33014
Nosy	@rhettinger, @terryjreedy, @bitdancer, @serhiy-storchaka, @willingc, @CuriousLearner, @csabella
PRs	bpo-33014: Clarify str.isidentifier docstring #6088 bpo-33014: minor docstring fixup #9756

^{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 2018-10-08.15:28:17.851>
created_at = <Date 2018-03-06.15:50:40.037>
labels = ['easy', '3.8', 'type-bug', '3.7', 'docs']
title = 'Clarify str.isidentifier docstring; fix keyword.iskeyword docstring'
updated_at = <Date 2018-10-08.15:28:17.849>
user = 'https://bugs.python.org/dabeaz'

bugs.python.org fields:

activity = <Date 2018-10-08.15:28:17.849>
actor = 'CuriousLearner'
assignee = 'docs@python'
closed = True
closed_date = <Date 2018-10-08.15:28:17.851>
closer = 'CuriousLearner'
components = ['Documentation']
creation = <Date 2018-03-06.15:50:40.037>
creator = 'dabeaz'
dependencies = []
files = []
hgrepos = []
issue_num = 33014
keywords = ['patch', 'easy']
message_count = 22.0
messages = ['313335', '313338', '313499', '313504', '313533', '313540', '313542', '313544', '313548', '313550', '313673', '313676', '313682', '313688', '313694', '313697', '313712', '314019', '314507', '314525', '327325', '327356']
nosy_count = 9.0
nosy_names = ['rhettinger', 'terry.reedy', 'r.david.murray', 'dabeaz', 'docs@python', 'serhiy.storchaka', 'willingc', 'CuriousLearner', 'cheryl.sabella']
pr_nums = ['6088', '9756']
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'behavior'
url = 'https://bugs.python.org/issue33014'
versions = ['Python 2.7', 'Python 3.6', 'Python 3.7', 'Python 3.8']

[dabeaz]

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 ;-).

[CuriousLearner]

Hey David,

I'm working on this issue. Will submit a PR in a while :)

[terryjreedy]

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'.

[dabeaz]

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 ;-)

[serhiy-storchaka]

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.

[terryjreedy]

I don't think my suggestion is much better either, and I otherwise agree with Serhiy. There are a thousand+ more important doc issues.

[dabeaz]

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.

[terryjreedy]

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."

[willingc]

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.

[terryjreedy]

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.

[CuriousLearner]

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](https://github.com/python/cpython/blob/main/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?

[serhiy-storchaka]

The change that will add a docstring to keyword.iskeyword() inevitable will have a negative performance effect. Is it worth it?

[terryjreedy]

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?

[serhiy-storchaka]

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.

[willingc]

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.

[csabella]

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

[terryjreedy]

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.

[bitdancer]

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".

[serhiy-storchaka]

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.

[bitdancer]

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.

[willingc]

New changeset ffc5a14 by Carol Willing (Sanyam Khurana) in branch 'master':
bpo-33014: Clarify str.isidentifier docstring (GH-6088)
ffc5a14

[CuriousLearner]

Marking this fixed via ffc5a14 and fc8205c

Thanks, everyone!