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

Improve repr for structseq objects to show named, but unindexed fields #55907

Open
rhettinger opened this issue Mar 27, 2011 · 14 comments
Open
Assignees
Labels
interpreter-core (Objects, Python, Grammar, and Parser dirs) type-feature A feature request or enhancement

Comments

@rhettinger
Copy link
Contributor

BPO 11698
Nosy @rhettinger, @pitrou, @ezio-melotti, @merwok, @ericsnowcurrently, @lilydjwg, @berkerpeksag, @serhiy-storchaka, @phmc, @mblahay
Files
  • issue11698.patch
  • structseq_2.patch: Show named but unindexed fields under the dict keyword argument
  • 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/pitrou'
    closed_at = None
    created_at = <Date 2011-03-27.22:14:56.506>
    labels = ['interpreter-core', 'easy', 'type-feature', '3.8']
    title = 'Improve repr for structseq objects to show named, but unindexed fields'
    updated_at = <Date 2020-10-19.08:47:32.922>
    user = 'https://github.com/rhettinger'

    bugs.python.org fields:

    activity = <Date 2020-10-19.08:47:32.922>
    actor = 'serhiy.storchaka'
    assignee = 'pitrou'
    closed = False
    closed_date = None
    closer = None
    components = ['Interpreter Core']
    creation = <Date 2011-03-27.22:14:56.506>
    creator = 'rhettinger'
    dependencies = []
    files = ['26391', '32527']
    hgrepos = []
    issue_num = 11698
    keywords = ['patch', 'easy']
    message_count = 14.0
    messages = ['132372', '153754', '153755', '155261', '165570', '188416', '200624', '200718', '202332', '202364', '224473', '341578', '341591', '378933']
    nosy_count = 13.0
    nosy_names = ['rhettinger', 'pitrou', 'ezio.melotti', 'eric.araujo', 'Arfrever', 'eric.snow', 'lilydjwg', 'berker.peksag', 'dlam', 'serhiy.storchaka', 'pconnell', 'sunfinite', 'mblahay']
    pr_nums = []
    priority = 'low'
    resolution = None
    stage = 'patch review'
    status = 'open'
    superseder = None
    type = 'enhancement'
    url = 'https://bugs.python.org/issue11698'
    versions = ['Python 3.8']

    @rhettinger
    Copy link
    Contributor Author

    The current __repr__ for structseq only shows the name/value pairs for the positional part and it ignores the other named fields.

    For example, os.stat(somefile) returns:
    posix.stat_result(st_mode=33277, st_ino=8468407, st_dev=234881026, st_nlink=1, st_uid=0, st_gid=80, st_size=25424, st_atime=1301263901, st_mtime=1298229258, st_ctime=1298283922)

    but it doesn't show the other named fields and their values:
    {'st_ctime': 1298283922.0, 'st_rdev': 0, 'st_mtime': 1298229258.0, 'st_blocks': 56, 'st_flags': 0, 'st_gen': 0, 'st_atime': 1301263901.0, 'st_blksize': 4096, 'st_birthtime': 1298229258.0}

    The __reduce__ method for structseq returns both the tuple portion and the dictionary portion. The latter needs to be added to the repr so that information doesn't get hidden from the user.

    @rhettinger rhettinger added interpreter-core (Objects, Python, Grammar, and Parser dirs) easy type-feature A feature request or enhancement labels Mar 27, 2011
    @berkerpeksag
    Copy link
    Member

    I'd like to work on this issue. I found the Objects/structseq.c [1] file. Am I on the right path?

    Thanks!

    [1] http://hg.python.org/cpython/file/5b4b70bd2b6f/Objects/structseq.c#l157

    @merwok
    Copy link
    Member

    merwok commented Feb 20, 2012

    You are! See the devguide for more setup guidelines.

    @pitrou
    Copy link
    Member

    pitrou commented Mar 9, 2012

    +1. Also, the repr() should show the float values of st_mtime and friends, rather than truncated integers.

    @dlam
    Copy link
    Mannequin

    dlam mannequin commented Jul 16, 2012

    hi hi, found this bug after clicking the "Easy issues" link

    i basically just took Ray's hint to look at the __reduce__ method, and applied it to the __repr__ method in this patch

    also updated is the test_repr() unittest

    @pitrou
    Copy link
    Member

    pitrou commented May 4, 2013

    Thanks for the patch. (Yes, I'm looking at this a bit late :-))

    First, there seems to be a problem with the repr() of of.stat() results:

    ./python -c "import os; print(os.stat('LICENSE'))"
    posix.stat_result(st_mode=33204, st_ino=6553619, st_dev=2053, st_nlink=1, st_uid=1000, st_gid=1000, st_size=15089, st_atime=1367693898, st_mtime=1365264866, st_ctime=1366481591, st_atime=1367693898.528636, st_mtime=1365264866.4163036, st_ctime=1366481591.9862735, st_atime_ns=1367693898528635928, st_mtime_ns=1365264866416303676, st_ctime_ns=1366481591986273627, st_blksize=4096, st_blocks=32, st_rdev=0)

    As you see, fields such as "st_atime" are duplicated.

    There are other issues with the patch:

    • C variable declarations should always be at the beginning of blocks (otherwise it's not C89-compliant)

    • C++-style comments (//) are forbidden

    • I don't understand in which circumstances Py_TYPE(obj)->tp_members[i-n_unnamed_fields].name can be NULL

    @sunfinite
    Copy link
    Mannequin

    sunfinite mannequin commented Oct 20, 2013

    Added patch for 3.4.

    The patch demarcates the output by adding a {...} around the dictionary portion. Please let me know if this is the right format or if not required at all. It is a simple change.

    @pitrou
    Copy link
    Member

    pitrou commented Oct 21, 2013

    Hmm, does anyone have an opinion for or against the proposed representation in Sunny's patch?

    Sunny, if you haven't done so, could you sign a contributor's agreement? http://www.python.org/psf/contrib/
    Thanks!

    @sunfinite
    Copy link
    Mannequin

    sunfinite mannequin commented Nov 7, 2013

    The previous patch had a wrong mapping between keys and values. The
    current implementation of repr means that duplicated keys will be
    present when invisible fields are included. See points 2 and 3 in
    http://bugs.python.org/issue1820#msg202330 for more explanation.

    I have sidestepped that issue by placing invisible fields under the dict argument. This also plays well with the current code in
    structseq_new and eval(repr(obj)) works.

    The output with the patch is:

    $./python -c "import os; print(os.stat('LICENSE'))"
    os.stat_result(st_mode=33188, st_ino=577299, st_dev=64512, st_nlink=1, st_uid=33616, st_gid=600, st_size=12749, st_atime=1382696747,
    st_mtime=1382361968, st_ctime=1382361968,
    dict={'st_atime':1382696747.0, 'st_mtime':1382361968.0,
    'st_ctime':1382361968.0, 'st_atime_ns':1382696747000000000,
    'st_mtime_ns':1382361968000000000, 'st_ctime_ns':1382361968000000000,
    'st_blksize':4096, 'st_blocks':32, 'st_rdev':0})

    @Arfrever
    Copy link
    Mannequin

    Arfrever mannequin commented Nov 7, 2013

    IMHO '*' could be used as a separator, since relation between indexable fields and named, unindexable fields is similar to relation between positional-or-keyword parameters and keyword-only parameters.

    $./python -c "import os; print(os.stat('LICENSE'))"
    os.stat_result(st_mode=33188, st_ino=577299, st_dev=64512, st_nlink=1, st_uid=33616, st_gid=600, st_size=12749, st_atime=1382696747, st_mtime=1382361968, st_ctime=1382361968, *, st_atime=1382696747.0, st_mtime=1382361968.0, st_ctime=1382361968.0, st_atime_ns=1382696747000000000, st_mtime_ns=1382361968000000000, st_ctime_ns=1382361968000000000, st_blksize=4096, st_blocks=32, st_rdev=0)

    @BreamoreBoy
    Copy link
    Mannequin

    BreamoreBoy mannequin commented Aug 1, 2014

    Could somebody pick this up please as it fixes bpo-5907.

    @abalkin abalkin added the 3.7 (EOL) end of life label Oct 3, 2016
    @rhettinger rhettinger added 3.8 only security fixes and removed 3.7 (EOL) end of life labels Jan 29, 2018
    @mblahay
    Copy link
    Mannequin

    mblahay mannequin commented May 6, 2019

    I will work on this

    @mblahay
    Copy link
    Mannequin

    mblahay mannequin commented May 6, 2019

    I have been advised to avoid enhancements like this one, so I am setting this back down. Also, this should be relabeled as easy(c).

    @serhiy-storchaka
    Copy link
    Member

    Other problem is that the repr looks like an evaluable expression, but evaluating it will always produce error.

    >>> st = os.stat('/dev/null')
    >>> st
    os.stat_result(st_mode=8630, st_ino=6, st_dev=6, st_nlink=1, st_uid=0, st_gid=0, st_size=0, st_atime=1602523313, st_mtime=1602523313, st_ctime=1602523313)
    >>> os.stat_result(st_mode=8630, st_ino=6, st_dev=6, st_nlink=1, st_uid=0, st_gid=0, st_size=0, st_atime=1602523313, st_mtime=1602523313, st_ctime=1602523313)
    Traceback (most recent call last):
      File "<stdin>", line 1, in <module>
    TypeError: structseq() takes at most 2 keyword arguments (10 given)

    os.stat_result() accepts only two arguments: a tuple for indexable elements and a dict for non-indexable elements.

    >>> os.stat_result((8630, 6, 6, 1, 0, 0, 0, 1602523313, 1602523313, 1602523313), {'st_atime': 1602523313.282834, 'st_mtime': 1602523313.282834, 'st_ctime': 1602523313.282834, 'st_atime_ns': 1602523313282834115, 'st_mtime_ns': 1602523313282834115, 'st_ctime_ns': 1602523313282834115, 'st_blksize': 4096, 'st_blocks': 0, 'st_rdev': 259})
    os.stat_result(st_mode=8630, st_ino=6, st_dev=6, st_nlink=1, st_uid=0, st_gid=0, st_size=0, st_atime=1602523313, st_mtime=1602523313, st_ctime=1602523313)

    But such form looks not very readable, because it lacks names for indexable elements.

    To solve this we can use an angular form in the repr:

    <os.stat_result ...>

    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    @erlend-aasland erlend-aasland removed easy 3.8 only security fixes labels Mar 3, 2024
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    interpreter-core (Objects, Python, Grammar, and Parser dirs) type-feature A feature request or enhancement
    Projects
    None yet
    Development

    No branches or pull requests

    7 participants