-
-
Notifications
You must be signed in to change notification settings - Fork 31.1k
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
bpo-30872: Update the curses docs to Python 3. #2620
Conversation
@serhiy-storchaka, thanks for your PR! By analyzing the history of the files in this pull request, we identified @ezio-melotti, @birkenfeld and @orsenthil to be potential reviewers. |
@@ -176,7 +176,7 @@ C library: | |||
|
|||
Checks for a non-ASCII character (ordinal values 0x80 and above). | |||
|
|||
These functions accept either integers or strings; when the argument is a | |||
These functions accept either integers or single-character strings; when the argument is a |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be changed? It accept multi-character strings, too. Alought it will raise TypeError
by ord
.
Also, after this paragraph, it has written this Note that all these functions check ordinal bit values derived from the first character of the string you pass in
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does single-character string
as same as you mention in note? (it can be specified as a Unicode string or a byte string.
). If yes, but these function can't accept byte string:
>>> curses.ascii.ascii(b'1')
Traceback (most recent call last):
File "<stdin>", line 1, in <module>
File "/usr/lib/python3.6/curses/ascii.py", line 75, in ascii
return _ctoi(c) & 0x7f
TypeError: unsupported operand type(s) for &: 'bytes' and 'int'
>>> curses.ascii.ctrl(b'1')
Traceback (most recent call last):
File "<stdin>", line 1, in <module>
File "/usr/lib/python3.6/curses/ascii.py", line 81, in ctrl
return _ctoi(c) & 0x1f
TypeError: unsupported operand type(s) for &: 'bytes' and 'int'
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Raising TypeError doesn't mean that the function accepts multi-character strings.
"string" means just string here. Although I think it is worth to add the support of bytes here, this PR changes only the documentation.
Doc/library/curses.rst
Outdated
Return the current coordinates of the virtual screen cursor in y and x. If | ||
leaveok is currently true, then -1,-1 is returned. | ||
Return the current coordinates of the virtual screen cursor as a tuple | ||
``(y, x)``. If **leaveok** is currently true, then ``(-1, -1)`` is returned. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
true should be cast to ``True``
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably it should be True
. Even if the module isn't actually using booleans internally, conceptually it is; leaveok can really only have boolean values.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
leaveok is a name of the window option. It is changed by the method window.leaveok()
. How better to reword this?
Doc/library/curses.rst
Outdated
If *yes* is 1, allow 8-bit characters to be input. If *yes* is 0, allow only | ||
7-bit chars. | ||
If an argument is ``True``, allow 8-bit characters to be input. If an | ||
argument is ``False``, allow only 7-bit chars. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the argument
Doc/library/curses.rst
Outdated
string is set to the value of cr. The effect is that the cursor is confined to | ||
called. The effect is that, during those calls, :envvar:`LINES` is set to ``1``; the | ||
capabilities **clear**, **cup**, **cud**, **cud1**, **cuu1**, **cuu**, **vpa** are disabled; and the **home** | ||
string is set to the value of **cr**. The effect is that the cursor is confined to |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Double asterisks mark up as bold don’t they? Perhaps italics (single asterisks) would be more appropriate.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I checked, and we use "bytes object" 98 times in library docs, and "byte string" 42 times. So I think we are shifting to using "bytes object". I would like to see all the instances of "byte string" changed to "bytes object" here.
Doc/library/curses.ascii.rst
Outdated
@@ -176,7 +176,7 @@ C library: | |||
|
|||
Checks for a non-ASCII character (ordinal values 0x80 and above). | |||
|
|||
These functions accept either integers or strings; when the argument is a | |||
These functions accept either integers or single-character strings; when the argument is a | |||
string, it is first converted using the built-in function :func:`ord`. | |||
|
|||
Note that all these functions check ordinal bit values derived from the first |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"from the first character of the string you pass in" seems confusing, given that the string may only have one character. Perhaps change it to "from the character in the string you pass in". (I realize this is an "existing bug", but we might as well make it more precise while we are editing the rest).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree. Thank you for the suggestion.
The rest of the paragraph looks outdated too. Most functions was moved from the string
module and become str
methods. And they don't know about the character encoding because handle Unicode.
Doc/library/curses.rst
Outdated
string is set to the value of cr. The effect is that the cursor is confined to | ||
called. The effect is that, during those calls, :envvar:`LINES` is set to ``1``; the | ||
capabilities **clear**, **cup**, **cud**, **cud1**, **cuu1**, **cuu**, **vpa** are disabled; and the **home** | ||
string is set to the value of **cr**. The effect is that the cursor is confined to |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I haven't seen two asterisks used before. What convention does this follow? I'm thinking these should be single *
s?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wanted to distinguish them from other names. These names are not the names of arguments (which are rendered as italic). They are the names of terminal capabilities. If you prefer, I can change them to be rendered as fixed-width (``clear``), but not as italic.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I think that would be better.
Doc/library/curses.rst
Outdated
Return the current coordinates of the virtual screen cursor in y and x. If | ||
leaveok is currently true, then -1,-1 is returned. | ||
Return the current coordinates of the virtual screen cursor as a tuple | ||
``(y, x)``. If **leaveok** is currently true, then ``(-1, -1)`` is returned. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably it should be True
. Even if the module isn't actually using booleans internally, conceptually it is; leaveok can really only have boolean values.
Doc/library/curses.rst
Outdated
If *yes* is 1, allow 8-bit characters to be input. If *yes* is 0, allow only | ||
7-bit chars. | ||
If an argument is ``True``, allow 8-bit characters to be input. If an | ||
argument is ``False``, allow only 7-bit chars. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s/an argument/*flag*/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I unified all boolean argument names (now all are "flag"), but removed names from descriptions because actually the name is arbitrary. Since all these functions take a single argument, there is no need to name it.
With these reasons what is better, "the argument
" or "*flag*
"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
*flag*
. Our documentation convention is that we refer to the arguments by the name given in the prototype, regardless of whether that is what is actually used in the code or not. It would be valid English to say "the argument", but I think consistency with the bulk of the docs is a good idea.
Doc/library/curses.rst
Outdated
Set the virtual screen cursor to *y*, *x*. If *y* and *x* are both -1, then | ||
leaveok is set. | ||
Set the virtual screen cursor to *y*, *x*. If *y* and *x* are both ``-1``, then | ||
**leaveok** is set. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this should be a link to the function, and it should probably say "set true".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If *y* and *x* are both ``-1``, then :meth:`~window.leaveok` set true.
Right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"is set true". Sorry I didn't make that clear.
Doc/library/curses.rst
Outdated
|
||
If called with *yes* equal to 1, :mod:`curses` will try and use hardware line | ||
If called with an argument equal to ``True``, :mod:`curses` will try and use hardware line |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s/an argument/*flag*/
Doc/library/curses.rst
Outdated
editing facilities. Otherwise, line insertion/deletion are disabled. | ||
|
||
|
||
.. method:: window.immedok(flag) | ||
|
||
If *flag* is ``True``, any change in the window image automatically causes the | ||
If an argument is ``True``, any change in the window image automatically causes the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
revert this
Doc/library/curses.rst
Outdated
If *yes* is 1, escape sequences generated by some keys (keypad, function keys) | ||
will be interpreted by :mod:`curses`. If *yes* is 0, escape sequences will be | ||
If an argument is ``True``, escape sequences generated by some keys (keypad, function keys) | ||
will be interpreted by :mod:`curses`. If an argument is ``False``, escape sequences will be |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s/an argument/*flag*/
Doc/library/curses.rst
Outdated
|
||
If *yes* is 1, cursor is left where it is on update, instead of being at "cursor | ||
If an argument is ``True``, cursor is left where it is on update, instead of being at "cursor |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s/an argument/*flag*/
Ok, I'm going to stop noting this every time, you get the idea :)
Doc/library/curses.rst
Outdated
@@ -1202,7 +1209,7 @@ the following methods and attributes: | |||
|
|||
.. method:: window.syncok(flag) | |||
|
|||
If called with *flag* set to ``True``, then :meth:`syncup` is called automatically | |||
If called with ``True``, then :meth:`syncup` is called automatically |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If *flag* is ``True``...
I intentionally used the term "byte string" for accenting that a bytes object is not just arbitrary couple of bits as in binary representation of floats or images, but mostly human-readable string. Do you think this makes sense? |
I still prefer bytes object, but in this context it is not a strong preference. The "character string" idiom is also somewhat problematic, but I don't have a better solution to suggest. |
Doc/library/curses.rst
Outdated
@@ -786,7 +787,7 @@ the following methods and attributes: | |||
window.chgat(y, x, num, attr) | |||
|
|||
Set the attributes of *num* characters at the current cursor position, or at | |||
position ``(y, x)`` if supplied. If no value of *num* is given or *num* = ``-1``, | |||
position ``(y, x)`` if supplied. If no value of *num* is given or ``-1`` is given, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why this change? I think the original read better. If you want to use words rather than code, you could write “or num is −1”, or “num is negative one”. Or perhaps “If num is not given or is −1”.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It was changed on the request by @bitdancer.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't particularly remember this, but I suspect my logic was that we don't usually write "arg = " when describing argument values, since that looks like code and thus is potentially confusing in this context. I like the "if num is not given or is -1" phrasing. Oddly enough, I think it might read better in the form "if num is -1 or is not given", but either one is fine with me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Either is fine by me
Doc/library/curses.rst
Outdated
range: function keys, keypad keys and so on return numbers higher than 256. In | ||
no-delay mode, ``-1`` is returned if there is no input, else the function waits | ||
until a key is pressed. | ||
range: function keys, keypad keys and so are represented by numbers higher |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would prefer to restore the word on. And so on means and similar cases.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch! I removed it by mistake when edited following words.
Should I do anything more @bitdancer? |
Ping. |
Thanks @serhiy-storchaka for the PR 🌮🎉.. I'm working now to backport this PR to: 3.6. |
@serhiy-storchaka, Something is wrong... I can't backport for now. |
(cherry picked from commit 300dd55)
GH-3887 is a backport of this pull request to the 3.6 branch. |
@Mariatta, running |
Thanks @serhiy-storchaka . It's strange; quickly reading the log, there's an exception when it was trying to do |
I don't know if it is relevant, but in the past few days on the commercial project I'm working on we've seen an increased incidence of codeship CI jobs failing because the pull from github fails, and then the restarted job will work fine. That is, this may be a github reliability issue. |
Thanks @bitdancer :) Yeah I just restarted miss-islington hoping to get it in clean state .. I think the next backport should work.. |
https://bugs.python.org/issue30872