| 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) *  |
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) *  |
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) * |
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) * |
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) * |
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) * |
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) * |
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) * |
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) * |
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) * |
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) * |
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) * |
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) * |
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) * |
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) * |
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) * |
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) * |
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) * |
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) * |
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! |
|
|