bpo-31680: Add curses.ncurses_version. #4217
Conversation
vstinner
left a comment
vstinner
left a comment
There was a problem hiding this comment.
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.
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.
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.
Please write "Available ..." in a new paragraph, to have a similar style than "Availability: ..." used in the os documentation.
vstinner
left a comment
vstinner
left a comment
There was a problem hiding this comment.
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) |
| PyDict_SetItemString(d, "__version__", v); | ||
| Py_DECREF(v); | ||
|
|
||
| #ifdef NCURSES_VERSION |
There was a problem hiding this comment.
Ah ok, now it's more explicit :-)
| @@ -0,0 +1 @@ | |||
| Added :data:`curses.ncurses_version`. | |||
There was a problem hiding this comment.
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. |
5 participants
https://bugs.python.org/issue31680