classification
Title: sys.getwindowsversion as PyStructSequence
Type: enhancement Stage: committed/rejected
Components: Library (Lib) Versions: Python 3.2, Python 2.7
process
Status: closed Resolution: accepted
Dependencies: Superseder:
Assigned To: eric.smith Nosy List: brian.curtin, eric.smith, lemburg
Priority: normal Keywords: needs review, patch

Created on 2010-01-23 19:57 by brian.curtin, last changed 2010-07-08 22:31 by brian.curtin. This issue is now closed.

Files
File name Uploaded Description Edit
winver_as_structseq.diff brian.curtin, 2010-01-23 19:57 Patch against trunk r77710
winver_as_structseq_ex.diff brian.curtin, 2010-01-23 21:14 use OSVERSIONINFOEX
winver_as_structseq_ex1.diff eric.smith, 2010-01-26 07:36
winver_as_structseq_ex2.diff eric.smith, 2010-01-26 13:54
winver_as_structseq_ex4.diff eric.smith, 2010-01-26 23:27 Proposed patch.
Messages (16)
msg98197 - (view) Author: Brian Curtin (brian.curtin) * (Python committer) Date: 2010-01-23 19:57
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.
msg98198 - (view) Author: Eric V. Smith (eric.smith) * (Python committer) Date: 2010-01-23 20:42
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.
msg98200 - (view) Author: Brian Curtin (brian.curtin) * (Python committer) Date: 2010-01-23 21:14
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.
msg98318 - (view) Author: Eric V. Smith (eric.smith) * (Python committer) Date: 2010-01-26 07:36
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.
msg98319 - (view) Author: Eric V. Smith (eric.smith) * (Python committer) Date: 2010-01-26 07:58
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.
msg98323 - (view) Author: Marc-Andre Lemburg (lemburg) * (Python committer) Date: 2010-01-26 10:07
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.
msg98330 - (view) Author: Eric V. Smith (eric.smith) * (Python committer) Date: 2010-01-26 12:04
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.
msg98332 - (view) Author: Marc-Andre Lemburg (lemburg) * (Python committer) Date: 2010-01-26 12:40
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.
msg98336 - (view) Author: Eric V. Smith (eric.smith) * (Python committer) Date: 2010-01-26 13:54
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.
msg98338 - (view) Author: Brian Curtin (brian.curtin) * (Python committer) Date: 2010-01-26 14:53
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.
msg98357 - (view) Author: Eric V. Smith (eric.smith) * (Python committer) Date: 2010-01-26 19:17
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.
msg98389 - (view) Author: Eric V. Smith (eric.smith) * (Python committer) Date: 2010-01-26 23:27
Here's the final version of the patch. After some testing on various platforms I'll commit it.
msg98395 - (view) Author: Eric V. Smith (eric.smith) * (Python committer) Date: 2010-01-27 00:46
Committed in trunk r77763, in py3k r77765.
msg109551 - (view) Author: Brian Curtin (brian.curtin) * (Python committer) Date: 2010-07-08 15:36
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.
msg109624 - (view) Author: Marc-Andre Lemburg (lemburg) * (Python committer) Date: 2010-07-08 22:25
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
msg109629 - (view) Author: Brian Curtin (brian.curtin) * (Python committer) Date: 2010-07-08 22:31
Yep, setting this back to closed.
History
Date User Action Args
2010-07-08 22:31:36brian.curtinsetstatus: open -> closed
keywords: patch, patch, needs review
messages: + msg109629
2010-07-08 22:25:34lemburgsetmessages: + msg109624
2010-07-08 15:36:21brian.curtinsetstatus: closed -> open
keywords: patch, patch, needs review
messages: + msg109551
2010-01-27 00:46:31eric.smithsetstatus: open -> closed
messages: + msg98395

assignee: eric.smith
keywords: patch, patch, needs review
resolution: accepted
stage: patch review -> committed/rejected
2010-01-26 23:27:06eric.smithsetkeywords: patch, patch, needs review
files: + winver_as_structseq_ex4.diff
messages: + msg98389
2010-01-26 19:17:06eric.smithsetkeywords: patch, patch, needs review

messages: + msg98357
2010-01-26 14:53:20brian.curtinsetkeywords: patch, patch, needs review

messages: + msg98338
2010-01-26 13:54:05eric.smithsetkeywords: patch, patch, needs review
files: + winver_as_structseq_ex2.diff
messages: + msg98336
2010-01-26 12:40:11lemburgsetmessages: + msg98332
2010-01-26 12:04:11eric.smithsetkeywords: patch, patch, needs review

messages: + msg98330
2010-01-26 10:07:56lemburgsetnosy: + lemburg
messages: + msg98323
2010-01-26 07:58:53eric.smithsetkeywords: patch, patch, needs review

messages: + msg98319
2010-01-26 07:36:20eric.smithsetkeywords: patch, patch, needs review
files: + winver_as_structseq_ex1.diff
messages: + msg98318
2010-01-23 21:14:39brian.curtinsetkeywords: patch, patch, needs review
files: + winver_as_structseq_ex.diff
messages: + msg98200
2010-01-23 20:42:30eric.smithsetkeywords: patch, patch, needs review
nosy: + eric.smith
messages: + msg98198

2010-01-23 19:57:10brian.curtincreate