Repository navigation
sys.getwindowsversion as PyStructSequence #52014
Description
| BPO | 7766 |
|---|---|
| Nosy | @malemburg, @ericvsmith, @briancurtin |
| Files |
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']
Activity
briancurtin commentedon Jan 23, 2010
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 commentedon Jan 23, 2010
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 commentedon Jan 23, 2010
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 commentedon Jan 26, 2010
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 commentedon Jan 26, 2010
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.
Or maybe we should make it a bool instead, and not worry about future expansion.
malemburg commentedon Jan 26, 2010
Eric Smith wrote:
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 commentedon Jan 26, 2010
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 commentedon Jan 26, 2010
Eric Smith wrote:
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 commentedon Jan 26, 2010
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 commentedon Jan 26, 2010
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 commentedon Jan 26, 2010
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 commentedon Jan 26, 2010
Here's the final version of the patch. After some testing on various platforms I'll commit it.
ericvsmith commentedon Jan 27, 2010
Committed in trunk r77763, in py3k r77765.
briancurtin commentedon Jul 8, 2010
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 commentedon Jul 8, 2010
Brian Curtin wrote:
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 commentedon Jul 8, 2010
Yep, setting this back to closed.