Skip to content
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

Merged
merged 6 commits into from
Oct 30, 2018

Conversation

serhiy-storchaka
Copy link
Member

@serhiy-storchaka serhiy-storchaka commented Nov 1, 2017

@serhiy-storchaka serhiy-storchaka added the type-feature A feature request or enhancement label Nov 1, 2017
Copy link
Member

@vstinner vstinner left a 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)!

self.assertEqual(v[0], v.major)
self.assertEqual(v[1], v.minor)
self.assertEqual(v[2], v.patch)
self.assertTrue(v > (0, 0, 0))
Copy link
Member

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

Copy link
Member Author

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.

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
Copy link
Member

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.

Copy link
Member

@vstinner vstinner left a 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)
Copy link
Member

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
Copy link
Member

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`.
Copy link
Member

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.

@taleinat
Copy link
Contributor

taleinat commented Sep 6, 2018

Ping, @serhiy-storchaka? This looks good to go!

@vstinner
Copy link
Member

vstinner commented Sep 6, 2018

Yep, this PR still LGTM.

@serhiy-storchaka
Copy link
Member Author

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.

@vstinner
Copy link
Member

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.

@serhiy-storchaka serhiy-storchaka merged commit b232df9 into python:master Oct 30, 2018
@serhiy-storchaka serhiy-storchaka deleted the ncurses_version branch October 30, 2018 11:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type-feature A feature request or enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants