Skip to content

tuple.index error message improvement #74662

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

Closed
llllllllll mannequin opened this issue May 25, 2017 · 7 comments
Closed

tuple.index error message improvement #74662

llllllllll mannequin opened this issue May 25, 2017 · 7 comments
Labels
3.7 (EOL) end of life interpreter-core (Objects, Python, Grammar, and Parser dirs)

Comments

@llllllllll
Copy link
Mannequin

llllllllll mannequin commented May 25, 2017

BPO 30477
Nosy @brettcannon, @rhettinger, @vadmium, @llllllllll
PRs
  • bpo-13349: repr object in tuple.index ValueError message #1820
  • Superseder
  • bpo-13349: Non-informative error message in index() and remove() functions
  • 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 = None
    closed_at = <Date 2017-05-27.19:40:12.157>
    created_at = <Date 2017-05-25.22:41:20.187>
    labels = ['interpreter-core', '3.7']
    title = 'tuple.index error message improvement'
    updated_at = <Date 2017-05-27.19:40:12.156>
    user = 'https://github.com/llllllllll'

    bugs.python.org fields:

    activity = <Date 2017-05-27.19:40:12.156>
    actor = 'brett.cannon'
    assignee = 'none'
    closed = True
    closed_date = <Date 2017-05-27.19:40:12.157>
    closer = 'brett.cannon'
    components = ['Interpreter Core']
    creation = <Date 2017-05-25.22:41:20.187>
    creator = 'llllllllll'
    dependencies = []
    files = []
    hgrepos = []
    issue_num = 30477
    keywords = []
    message_count = 7.0
    messages = ['294516', '294517', '294518', '294519', '294522', '294537', '294550']
    nosy_count = 5.0
    nosy_names = ['brett.cannon', 'rhettinger', 'martin.panter', 'llllllllll', 'schen']
    pr_nums = ['1820']
    priority = 'normal'
    resolution = 'duplicate'
    stage = 'resolved'
    status = 'closed'
    superseder = '13349'
    type = None
    url = 'https://bugs.python.org/issue30477'
    versions = ['Python 3.7']

    @llllllllll
    Copy link
    Mannequin Author

    llllllllll mannequin commented May 25, 2017

    The old error of tuple.index(x): x not in tuple seemed very confusing to me. It was also harder to quickly understand what the program was doing wrong. This saves people a second pass through the program under the debugger because they can just see what the invalid value was.

    The reason I am explicitly calling repr instead of using %R is that I didn't want to call repr twice if I didn't need to. If people think that is fine the format can just be tuple.index(%R): %R not in tuple instead.

    @llllllllll llllllllll mannequin added the 3.7 (EOL) end of life label May 25, 2017
    @brettcannon
    Copy link
    Member

    I'm wondering if including the whole "tuple.index(%R)" part is still necessary? The traceback will tell you what method you called, so wouldn't "%R not in tuple" be enough?

    @brettcannon brettcannon added interpreter-core (Objects, Python, Grammar, and Parser dirs) type-feature A feature request or enhancement labels May 25, 2017
    @llllllllll
    Copy link
    Mannequin Author

    llllllllll mannequin commented May 25, 2017

    I agree, "%R in tuple" is enough information for me. This would also remove the need to manually repr the object.

    @llllllllll llllllllll mannequin removed the type-feature A feature request or enhancement label May 25, 2017
    @llllllllll
    Copy link
    Mannequin Author

    llllllllll mannequin commented May 25, 2017

    As a more meta question: I have noticed that many error messages in the stdlib use PyErr_SetString, or choose to use the type name instead of the repr of the object. Is there a reason for this? I normally try to fill the error message with as much context as possible to make debugging easier. Is it a performance worry?

    @rhettinger
    Copy link
    Contributor

    Is it a performance worry?

    It really depends on what people are doing with their down. If indexing potentially missing values is common, there will be a performance impact.

    Also, the cost of a __repr__ varies wildly depending on the data (i.e. a collections.Counter instance has an expensive __repr__, decimal objects have computations to runs as well).

    From a user point of view, a repr might be helpful or it might be disasterously lengthy (a webpage, a giant bytearray, a long list of tuples).

    Personally, I would rather not do this PR which seems to have the view that the exception is an error condition as opposed to being a normal way to terminate a series of index() calls.

    @schen
    Copy link
    Mannequin

    schen mannequin commented May 26, 2017

    What is the rationale for the inconsistency between tuples and lists here?

    ().index(0)
    ValueError: tuple.index(x): x not in tuple

    [].index(0)
    ValueError: 0 is not in list

    In this simple artificial case, it seems clear to me that the list version (which is similar to the proposed change) is superior.

    However, it is often not sufficient to use such cases as the basis for making a decision. Raymond raises some very good points.

    In particular, I noticed that printing the repr quickly becomes unwieldy for more complex objects. For example, I tried the following:
    [].index(''.join(chr(randrange(97, 97+26)) for _ in range(1000)))
    [].index(Counter(randrange(10) for _ in range(10000)))

    @vadmium
    Copy link
    Member

    vadmium commented May 26, 2017

    Also discussed in bpo-13349

    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    3.7 (EOL) end of life interpreter-core (Objects, Python, Grammar, and Parser dirs)
    Projects
    None yet
    Development

    No branches or pull requests

    3 participants