sys.getwindowsversion as PyStructSequence

[briancurtin]

BPO	7766
Nosy	@malemburg, @ericvsmith, @briancurtin
Files	winver_as_structseq.diff: Patch against trunk r77710 winver_as_structseq_ex.diff: use OSVERSIONINFOEX winver_as_structseq_ex1.diff winver_as_structseq_ex2.diff winver_as_structseq_ex4.diff: Proposed patch.

^{Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.}

Show more details

GitHub fields:

assignee = 'https://github.com/ericvsmith'
closed_at = <Date 2010-07-08.22:31:36.292>
created_at = <Date 2010-01-23.19:57:10.590>
labels = ['type-feature', 'library']
title = 'sys.getwindowsversion as PyStructSequence'
updated_at = <Date 2010-07-08.22:31:36.291>
user = 'https://github.com/briancurtin'

bugs.python.org fields:

activity = <Date 2010-07-08.22:31:36.291>
actor = 'brian.curtin'
assignee = 'eric.smith'
closed = True
closed_date = <Date 2010-07-08.22:31:36.292>
closer = 'brian.curtin'
components = ['Library (Lib)']
creation = <Date 2010-01-23.19:57:10.590>
creator = 'brian.curtin'
dependencies = []
files = ['15982', '15984', '16006', '16012', '16020']
hgrepos = []
issue_num = 7766
keywords = ['patch', 'needs review']
message_count = 16.0
messages = ['98197', '98198', '98200', '98318', '98319', '98323', '98330', '98332', '98336', '98338', '98357', '98389', '98395', '109551', '109624', '109629']
nosy_count = 3.0
nosy_names = ['lemburg', 'eric.smith', 'brian.curtin']
pr_nums = []
priority = 'normal'
resolution = 'accepted'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'enhancement'
url = 'https://bugs.python.org/issue7766'
versions = ['Python 2.7', 'Python 3.2']

[briancurtin]

I always find myself wishing sys.getwindowsversion() utilized the named tuple concept, so here it is against trunk. sys.version_info was also changed in this manner for 2.7.

Because it is a PyStructSeq/named tuple, it is still accessible like a regular old tuple, but can now be accessed by named attributes.

One thing I don't like is that this is a function, unlike sys.version_info. I think something like sys.windows_version would be better...is there sense in making that an additional API and starting to phase out the getwindowsversion function in py3k?

The patch includes doc and test changes.

[ericvsmith]

I like this. I've visually reviewed the patch, but haven't tested it yet. I'm willing to commit this.

Could you add to the tests to assert that .major is equal to [0], etc.?

Also, the documentation says that element [4] is "text", but you've referred to it as "service_pack". I don't think "service_pack" is documented anywhere, but is clearly a better name than "text". The documentation of OSVERSIONINFO describes this as szCSDVersion, with the description of 'A null-terminated string, ..., that indicates the latest Service Pack installed on the system ...', so "service_pack" is okay with me. Can you change the documentation to refer to this field as "service_pack"?

Another idea would be to expose OSVERSIONINFOEX. I've personally wanted to get access to wProductType in the past. Any thoughts on that? I think OSVERSIONINFOEX would be available on any version of Windows that we care about going forward.

I don't see any point in changing it from a function to a property at this point. There's the hassle of migrating to the new version, and it's wrapping a Win32 API call anyway.

[briancurtin]

Good point about OSVERSIONINFOEX. I've actually wanted some of that info as well, and according to MSDN the minimum supported client to get that structure is Windows 2000 - same as OSVERSIONINFO.

Attached is a patch updated with your comments plus the use of OSVERSIONINFOEX.

[ericvsmith]

Here's an updated patch. I fixed some docstrings, modified it to work with the most recent assertIsInstance changes, and added #ifdef for Windows.

There are a number of test failures still, I think all of them relating to errors in platform.py where it calls sys.getwindowsversion() and unpacks the result. I'll look at those soon.

[ericvsmith]

The more I think about this, the more concerned I am about changing the number of elements in the tuple. That's the change that broke platform.py. Maybe we should add a parameter named something like "level", defaulting to 0.

0 = existing behavior, but with named tuple
1 = return named 9-tuple OSVERSIONINFOEX values
other values: reserved for future use

Or maybe we should make it a bool instead, and not worry about future expansion.

[malemburg]

Eric Smith wrote:

Eric Smith <eric@trueblade.com> added the comment:

The more I think about this, the more concerned I am about changing the number of elements in the tuple. That's the change that broke platform.py. Maybe we should add a parameter named something like "level", defaulting to 0.

0 = existing behavior, but with named tuple
1 = return named 9-tuple OSVERSIONINFOEX values
other values: reserved for future use

Or maybe we should make it a bool instead, and not worry about future expansion.

The usual approach to such problems is keeping the number of tuple
items and their order the same and only add new items as additional
attributes to the struct.

See the CodecInfo tuple in codecs.py for an example on how this is
done. The tuple is still a 4-tuple, but it provides access to more
items via named attributes.

[ericvsmith]

Great idea, Marc-Andre. I agree that's the better approach.

It looks like PyStructSequence supports this, by setting n_in_sequence to a value smaller then the number of PyStructSequence_Fields. A quick look doesn't show any uses of this in the C code (except maybe os.stat), but I'll investigate and make sure that's a supported use case.

[malemburg]

Eric Smith wrote:

Eric Smith <eric@trueblade.com> added the comment:

Great idea, Marc-Andre. I agree that's the better approach.

It looks like PyStructSequence supports this, by setting n_in_sequence to a value smaller then the number of PyStructSequence_Fields. A quick look doesn't show any uses of this in the C code (except maybe os.stat), but I'll investigate and make sure that's a supported use case.

If not, I'd suggest to move the code to Python, e.g. add a
class to the new sysinfo.py module, and then instantiate it via a C
call.

[ericvsmith]

Here's a patch that implement's Marc-Andre's suggestion. The docstring and documentation need work. I still need to verify that this isn't a misuse of PyStructSequence, but the tests pass on Windows. I need to verify a few other platforms to make sure the #ifdef logic is correct.

[briancurtin]

Thanks a lot for taking a look at this, Eric and Marc-Andre. Apologies for the few mistakes in there, I jumped the gun and submitted that last patch before having run the full test suite again.

Good catch on the missing #ifdef. I will also run this on Mac and Linux today and make sure nothing slips by, but it looks to be fine.

[ericvsmith]

I can confirm the most recent patch (ex2) doesn't break anything on MacOS or Linux.

It's clear that structseq.c is designed to be used this way, with more named members than unnamed members (and vice-versa, actually). See n_members vs. n_in_sequence in Objects/structseq.c.

I'll work on the docstring and documentation then I'll commit it.

[ericvsmith]

Here's the final version of the patch. After some testing on various platforms I'll commit it.

[ericvsmith]

Committed in trunk r77763, in py3k r77765.

[briancurtin]

The previously mentioned comments about backwards incompatibility with the number of items in the sequence are now a problem, since structseq now inherits from tuple. It seems that n_in_sequence gets ignored and we have a 9 item tuple.

[malemburg]

Brian Curtin wrote:

Brian Curtin <curtin@acm.org> added the comment:

The previously mentioned comments about backwards incompatibility with the number of items in the sequence are now a problem, since structseq now inherits from tuple. It seems that n_in_sequence gets ignored and we have a 9 item tuple.

But that's not a problem with this ticket, is it. The previous use
case has to be restored (after all, this was the main reason for
adding structseq years ago).

Someone goofed when making the said change to structseq. Is there a
ticket open for this ? Here's the change:
http://svn.python.org/view?view=rev&revision=82636

After some digging: Looks like it's being dealt with on
http://bugs.python.org/issue8413

[briancurtin]

Yep, setting this back to closed.