bpo-30101: Add support for curses.A_ITALIC. #1015

Eijebong · 2017-04-06T16:16:03Z

No description provided.

the-knights-who-say-ni · 2017-04-06T16:16:06Z

Hello, and thanks for your contribution!

I'm a bot set up to make sure that the project can legally accept your contribution by verifying you have signed the PSF contributor agreement (CLA).

Unfortunately we couldn't find an account corresponding to your GitHub username on bugs.python.org (b.p.o) to verify you have signed the CLA. This is necessary for legal reasons before we can look at your contribution. Please follow the steps outlined in the CPython devguide to rectify this issue.

Thanks again to your contribution and we look forward to looking at it!

berkerpeksag

Thanks for the PR, but we need two more things to move this forward:

Create an issue on bugs.python.org and update the PR title to include the issue number
Document A_ITALIC in Doc/library/curses.rst (additionally with a .. versionadded:: 3.7 markup)

berkerpeksag · 2017-04-19T13:30:07Z

Modules/_cursesmodule.c

@@ -3335,6 +3335,7 @@ PyInit__curses(void)
    SetDictInt("A_BLINK",               A_BLINK);
    SetDictInt("A_DIM",                 A_DIM);
    SetDictInt("A_BOLD",                A_BOLD);
+    SetDictInt("A_ITALIC",              A_ITALIC);


A_ITALIC is rather a new addition so I think we should conditionally set it:

#ifdef A_ITALIC SetDictInt("A_ITALIC", A_ITALIC); #endif

Eijebong · 2017-04-19T17:35:29Z

What should I prefix with .. versionadded:: 3.7 ?
The first or the second column ?

berkerpeksag · 2017-04-20T00:23:46Z

Good question. I'd add the following after the first table at https://docs.python.org/3/library/curses.html#constants:

.. versionadded:: 3.7
   ``A_ITALIC`` was added.

Of course, A_ITALIC still needs to be added to the first table.

zhangyangyu · 2017-04-25T08:03:12Z

Modules/_cursesmodule.c

@@ -3335,6 +3335,9 @@ PyInit__curses(void)
    SetDictInt("A_BLINK",               A_BLINK);
    SetDictInt("A_DIM",                 A_DIM);
    SetDictInt("A_BOLD",                A_BOLD);
+#ifdef A_ITALIC
+    SetDictInt("A_ITALIC",              A_ITALIC);
+#endif


A_ITALIC is a ncurses extension, not X/Open standard. So I think it should be put after A_VERTICAL and prefixed with a line /* ncurses extension */.

Please don't start to "document" constants here, it's not the right place.

zhangyangyu · 2017-04-25T08:05:18Z

Doc/library/curses.rst

@@ -1294,6 +1296,9 @@ Several constants are available to specify character cell attributes:
 | ``A_UNDERLINE``  | Underline mode.               |
 +------------------+-------------------------------+

+.. versionadded:: 3.7
+    ``A_ITALIC`` was added.
+


I am not sure this doc change is suitable. This list lacks many options and seems only list some strict sysv options here.

Where should I document this then ?

Honestly speaking I prefer to leave this out (no change) in this issue and just change the code. The documentation should belong to another issue, whether list all options and document their limitations, or clarifying these are some commonly available ones.

@zhangyangyu can you please stop mixing things up? I don't even know what other options are you talking about. This PR is about exposing the A_ITALIC constant. Please open a new issue to discuss your ideas on curses documentation on bugs.python.org.

The Python stdlib is full of things that are only available in specific platforms and that doesn't stop us from documenting them. See the os, sys, select modules for example. If you want to add a note that says "some of these constants may not be available in your choice of curses implementation", that's fine and we can add it in this PR.

@berkerpeksag if you don't know what other options you should first read them. I have said in my comment the documentation problem needs another issue, not in this one.

Adding the note as you said is not suitable. Why do you only document A_ITALIC not other options? e.g. A_LEFT, A_LOW. A_ITALIC should be solved with them but not singly added which IMHO leads to confusion.

So of course we need to document A_ITALIC. But instead of singly added it in this issue, We could either opening an issue solving the documentation problem first, make this issue dependent on it, or only changing code in this issue and solve the documentation in another issue once. Considering our pace I prefer the latter choice.

I don't really want to keep this simple PR open for months or merge it without documentation changes because you've invented a new problem. I don't see any previous documentation issues relevant to this hypothetical problem on bugs.p.o. Use bugs.p.o for your random ramblings, and keep code review comments specific to the issue.

@zhangyangyu: "I am not sure this doc change is suitable. This list lacks many options and seems only list some strict sysv options here."

I concur with @berkerpeksag: while your comment is correct, we should not block this change, please open a new issue to request to complete/update the documentation.

@berkerpeksag

@berkerpeksag: "Thanks for the PR, but we need two more things to move this forward: (...)" => done, I update the status of the PR

berkerpeksag · 2017-04-26T14:37:25Z

Doc/library/curses.rst

@@ -1294,6 +1296,9 @@ Several constants are available to specify character cell attributes:
 | ``A_UNDERLINE``  | Underline mode.               |
 +------------------+-------------------------------+

+.. versionadded:: 3.7
+    ``A_ITALIC`` was added.


Style nit: Please use three spaces:

.. versionadded:: 3.7 ``A_ITALIC`` was added.

berkerpeksag · 2017-04-26T14:39:32Z

done, I update the status of the PR

We now only need a Misc/NEWS entry :) @Eijebong do you have some time to add it?

Eijebong · 2017-04-26T14:40:44Z

Sure, I'll do it ^^

Edit: Done
Re-edit: Erf, I'll rebase
Re-re-edit: Done

zhangyangyu · 2017-04-26T14:49:09Z

@Eijebong ，if your name is not in MISC/ACKs, also need to add it.

bradwood · 2018-11-22T18:48:28Z

This appears to not be working on the my build

(yappt-PAYrWqqE) ✘-1 [brad@bradmac:~/Code/yappt] [master ↑·1|✚ 2…4] $ python
Python 3.7.0 (default, Jul 23 2018, 20:22:55)
[Clang 9.1.0 (clang-902.0.39.2)] on darwin
Type "help", "copyright", "credits" or "license" for more information.
>>> import curses
>>> curses.A_ITALIC
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
AttributeError: module 'curses' has no attribute 'A_ITALIC'
>>>

zhangyangyu · 2018-11-23T06:37:38Z

@bradwood The option is not always available. It depends on your underlying curses implementation library. For example, ncurses add the option in version 6.0, see https://invisible-island.net/ncurses/announce-6.0.html, so on my environment with a ncurses 5.0, it's not available.

bradwood · 2018-11-24T11:16:05Z

Ok..
Makes sense...

Having now upgraded the curses on my Mac to 6.1, how do I make python see the latest curses library? Do I need to rebuild python pointing to those headers, or can I just set an environment variable? like LDFLAGS or some such?

zhangyangyu · 2018-11-26T08:12:21Z

@bradwood I think you need recompile.

the-knights-who-say-ni added the CLA not signed label Apr 6, 2017

berkerpeksag previously requested changes Apr 19, 2017

View reviewed changes

Eijebong force-pushed the master branch from 7a6a50b to a629d1c Compare April 20, 2017 00:35

the-knights-who-say-ni added CLA signed and removed CLA not signed labels Apr 20, 2017

Eijebong changed the title ~~Add support for curses.A_ITALIC.~~ bpo-30101: Add support for curses.A_ITALIC. Apr 20, 2017

zhangyangyu reviewed Apr 25, 2017

View reviewed changes

vstinner approved these changes Apr 26, 2017

View reviewed changes

berkerpeksag reviewed Apr 26, 2017

View reviewed changes

Eijebong force-pushed the master branch 2 times, most recently from 8f4ce7a to f40ac69 Compare April 26, 2017 14:48

bpo-30101: Add support for curses.A_ITALIC.

16c511b

Eijebong force-pushed the master branch from f40ac69 to 16c511b Compare April 26, 2017 14:54

zhangyangyu merged commit ab7886b into python:master Apr 26, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

bpo-30101: Add support for curses.A_ITALIC. #1015

bpo-30101: Add support for curses.A_ITALIC. #1015

Eijebong commented Apr 6, 2017

the-knights-who-say-ni commented Apr 6, 2017

berkerpeksag left a comment

berkerpeksag Apr 19, 2017

Eijebong commented Apr 19, 2017

berkerpeksag commented Apr 20, 2017

zhangyangyu Apr 25, 2017

vstinner Apr 26, 2017

zhangyangyu Apr 25, 2017

Eijebong Apr 25, 2017

zhangyangyu Apr 25, 2017

berkerpeksag Apr 25, 2017

zhangyangyu Apr 26, 2017 •

edited

Loading

berkerpeksag Apr 26, 2017

vstinner Apr 26, 2017

berkerpeksag Apr 26, 2017

berkerpeksag commented Apr 26, 2017

Eijebong commented Apr 26, 2017 •

edited

Loading

zhangyangyu commented Apr 26, 2017

bradwood commented Nov 22, 2018

zhangyangyu commented Nov 23, 2018

bradwood commented Nov 24, 2018

zhangyangyu commented Nov 26, 2018

bpo-30101: Add support for curses.A_ITALIC. #1015

bpo-30101: Add support for curses.A_ITALIC. #1015

Conversation

Eijebong commented Apr 6, 2017

the-knights-who-say-ni commented Apr 6, 2017

berkerpeksag left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Eijebong commented Apr 19, 2017

berkerpeksag commented Apr 20, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zhangyangyu Apr 26, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

berkerpeksag commented Apr 26, 2017

Eijebong commented Apr 26, 2017 • edited Loading

zhangyangyu commented Apr 26, 2017

bradwood commented Nov 22, 2018

zhangyangyu commented Nov 23, 2018

bradwood commented Nov 24, 2018

zhangyangyu commented Nov 26, 2018

zhangyangyu Apr 26, 2017 •

edited

Loading

Eijebong commented Apr 26, 2017 •

edited

Loading