-
-
Notifications
You must be signed in to change notification settings - Fork 31.4k
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-31680: Add curses.ncurses_version. #4217
bpo-31680: Add curses.ncurses_version. #4217
Conversation
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 understand when curses.ncurses_version is not available: it seems to always be defined in the C code?
I like tuple-like API of curses.ncurses_version, it's easy to write curses.ncurses_version >= (5, 7)!
Lib/test/test_curses.py
Outdated
self.assertEqual(v[0], v.major) | ||
self.assertEqual(v[1], v.minor) | ||
self.assertEqual(v[2], v.patch) | ||
self.assertTrue(v > (0, 0, 0)) |
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 check that each field is > 0.
self.assertGreater(v.major, 0)
self.assertGreater(v.minor, 0)
self.assertGreater(v.patch, 0)
> (0, 0, 0)
test is not enough:
>>> (1, -2, 3) > (0, 0, 0)
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.
v.minor is 0 in the actual version.
Doc/library/curses.rst
Outdated
A named tuple containing the three components of the ncurses library | ||
version: *major*, *minor*, and *patch*. All values are integers. The | ||
components can also be accessed by name, so ``curses.ncurses_version[0]`` | ||
is equivalent to ``curses.ncurses_version.major`` and so on. Available |
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.
Please write "Available ..." in a new paragraph, to have a similar style than "Availability: ..." used in the os documentation.
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.
LGTM. I just proposed to mention the new feature in What's New in Python 3.7.
self.assertEqual(v[2], v.patch) | ||
self.assertGreaterEqual(v.major, 0) | ||
self.assertGreaterEqual(v.minor, 0) | ||
self.assertGreaterEqual(v.patch, 0) |
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.
Perfect, thank you :-)
@@ -3361,6 +3414,30 @@ PyInit__curses(void) | |||
PyDict_SetItemString(d, "__version__", v); | |||
Py_DECREF(v); | |||
|
|||
#ifdef NCURSES_VERSION |
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.
Ah ok, now it's more explicit :-)
@@ -0,0 +1 @@ | |||
Added :data:`curses.ncurses_version`. |
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.
You may add it to Doc/whatsnew/3.7.rst as well. It's up to you.
Ping, @serhiy-storchaka? This looks good to go! |
Yep, this PR still LGTM. |
I wanted first to add similar features in other modules which are interfaces to external libraries: zlib, expat, sqlite, tkinter, etc. This may lead to changing this PR for unifying with other versions. |
The API is fine. I don't see why the API would change. If you would like to refactor the implementation, that can be done later. I suggest to merge this PR. |
https://bugs.python.org/issue31680