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

Jump to bottom

Use a named tuple for sys.version_info #48535

Closed
brettcannon opened this issue Nov 9, 2008 · 16 comments
Closed

Use a named tuple for sys.version_info #48535

brettcannon opened this issue Nov 9, 2008 · 16 comments
Assignees
@ericvsmith
Labels
easy extension-modules C modules in the Modules dir type-bug An unexpected behavior, bug, or error

Comments

@brettcannon
Copy link
Member

BPO 4285
Nosy @loewis, @brettcannon, @rhettinger, @ncoghlan, @ericvsmith, @tiran
Files
  • patch-4285b.diff: Second draft of bug patch
  • patch-4285c.diff: Third draft of patch
  • patch-4285d.diff: Fourth draft of 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 2009-02-06.01:33:27.177>
created_at = <Date 2008-11-09.00:04:51.883>
labels = ['extension-modules', 'easy', 'type-bug']
title = 'Use a named tuple for sys.version_info'
updated_at = <Date 2009-02-06.01:33:27.176>
user = 'https://github.com/brettcannon'

    bugs.python.org fields:

    activity = <Date 2009-02-06.01:33:27.176>
actor = 'eric.smith'
assignee = 'eric.smith'
closed = True
closed_date = <Date 2009-02-06.01:33:27.177>
closer = 'eric.smith'
components = ['Extension Modules']
creation = <Date 2008-11-09.00:04:51.883>
creator = 'brett.cannon'
dependencies = []
files = ['12857', '12860', '12870']
hgrepos = []
issue_num = 4285
keywords = ['patch', 'easy']
message_count = 16.0
messages = ['75647', '75655', '80521', '80526', '80527', '80530', '80534', '80538', '80557', '80599', '81139', '81148', '81149', '81169', '81172', '81245']
nosy_count = 8.0
nosy_names = ['loewis', 'brett.cannon', 'rhettinger', 'exarkun', 'ncoghlan', 'eric.smith', 'christian.heimes', 'ross.light']
pr_nums = []
priority = 'low'
resolution = 'accepted'
stage = 'needs patch'
status = 'closed'
superseder = None
type = 'behavior'
url = 'https://bugs.python.org/issue4285'
versions = ['Python 3.1', 'Python 2.7']
    @brettcannon
    Copy link
    Member Author

    sys.version_info is just asking for a named tuple consisting of major,
    minor, micro, releaselevel, and serial.

    This is assuming, of course, that bootstrapping doesn't get in the way.

    @brettcannon brettcannon self-assigned this Nov 9, 2008
    @brettcannon brettcannon added extension-modules C modules in the Modules dir easy type-bug An unexpected behavior, bug, or error labels Nov 9, 2008
    @tiran
    Copy link
    Member

    tiran commented Nov 9, 2008

    I concur that bootstrapping may be a problem. Using a NamedTuple also
    increases the number of loaded modules by 4 (_collections.so, keyword.py
    and operator.so).

    But we could reimplement it with a PyStructSequence like I did for
    sys.float_info. It's straight forward and easy to implement with the
    example code in Object/floatobject.c:PyFloat_GetInfo().

    @rosslight
    Copy link
    Mannequin

    rosslight mannequin commented Jan 25, 2009

    Hello, my name is Ross Light. I've written a patch for this, but this
    is my first patch, so someone please review.

    This does pass all regression tests, but I did have to modify the
    test_sys case to not check for sys.version_info being a tuple.

    @exarkun
    Copy link
    Mannequin

    exarkun mannequin commented Jan 25, 2009

    You also need to add unit tests for the new behavior you've implemented.

    @rosslight
    Copy link
    Mannequin

    rosslight mannequin commented Jan 25, 2009

    Oh yes, you're right. Sorry!

    @rhettinger
    Copy link
    Contributor

    +1 on this idea

    @loewis
    Copy link
    Mannequin

    loewis mannequin commented Jan 25, 2009

    A couple of further comments:

    • please use tabs for indentation consistently.
    • please change Doc/library/sys.rst, including adding a versionchanged
      indication
    • I find the naming of the macros *Flag confusing; to me, a "flag" says
      "boolean" - but I'm not a native speaker (of English). Common names are
      "item", "field", "element".

    @rosslight
    Copy link
    Mannequin

    rosslight mannequin commented Jan 25, 2009

    Okay, here's a patch with the requested changes. You're right in saying
    that flag is usually boolean, I was just going along with several other
    files where there's float and int flags (i.e. floatobject.c).

    @ncoghlan
    Copy link
    Contributor

    Rather than deleting the isinstance() check from the tests completely, I
    suggest changing it to be:

    self.assert_(isinstance(vi[:], tuple))

    Also, comparing directly with a tuple is also a fairly common use of
    version_info so it would be worth adding a test to explicitly guarantee
    that comparison:

    self.assert_(vi > (1,0,0))

    Patch applied and built cleanly for me, but I haven't checked the doc
    build yet.

    @rosslight
    Copy link
    Mannequin

    rosslight mannequin commented Jan 26, 2009

    Tests added and new patch uploaded. Anything else, anyone?

    @ericvsmith ericvsmith assigned ericvsmith and unassigned brettcannon Feb 3, 2009
    @ericvsmith
    Copy link
    Member

    The doc string for sys includes:
    version_info -- version information as a tuple

    I'm not sure changing this to "... as a structseq" makes it any more
    useful, but it's more correct. Does anyone have a preference? I'd use
    the same wording as float_info, but that's missing from the doc string
    (and I'll deal with that as a separate issue).

    Other than that, this all looks good to me. I also tested that the docs
    build. I'll check it in once I get or invent new wording for the doc string.

    @brettcannon
    Copy link
    Member Author

    On Wed, Feb 4, 2009 at 05:56, Eric Smith <report@bugs.python.org> wrote:

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

    The doc string for sys includes:
    version_info -- version information as a tuple

    I'm not sure changing this to "... as a structseq" makes it any more
    useful, but it's more correct. Does anyone have a preference?

    Does "... as a named tuple" make sense?

    -Brett

    @ericvsmith
    Copy link
    Member

    "... as a named tuple" works for me. I'll go with that. Thanks!

    @tiran
    Copy link
    Member

    tiran commented Feb 4, 2009

    Technically it's not a named tuple. Calling it named tuple may cause
    confusing. http://docs.python.org/library/os.html#os.stat calls it a
    structure.

    @rhettinger
    Copy link
    Contributor

    Eric Smith <eric@trueblade.com> added the comment:
    "... as a named tuple" works for me. I'll go with that. Thanks!

    +1

    Remember, "named tuple" is a concept, not a class. It is anything that
    provides attribute access as an alternative to indexed access (see the
    definition in the glossary where time.struct_time is given as an
    example). Running the collections.named_tuple() factory function
    creates a new class with named tuple features, but it is just one of
    several ways of creating named tuples.

    @ericvsmith
    Copy link
    Member

    Committed in r69331 (trunk) and r69346 (py3k).

    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Assignees

    @ericvsmith ericvsmith

    Labels
    easy extension-modules C modules in the Modules dir type-bug An unexpected behavior, bug, or error
    Projects
    None yet
    Milestone
    No milestone
    Development

    No branches or pull requests
    5 participants
    @brettcannon @tiran @ericvsmith @ncoghlan @rhettinger