Skip to content

gh-121288: improve error messages for tuple.index, list.index and list.remove #121308

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
wants to merge 7 commits into from

Conversation

picnixz
Copy link
Member

@picnixz picnixz commented Jul 3, 2024

The formulation can be changed if you think a smaller message is needed. I put the method in the exception message because it's not always clear whether the traceback is available or not. At least, you will know which method caused the issue but I can remove it if you think it's too much.

@picnixz
Copy link
Member Author

picnixz commented Jul 3, 2024

Oh... we don't have an index for those methods? https://docs.python.org/3/tutorial/datastructures.html#more-on-lists

.. method:: list.index(x[, start[, end]])
   :noindex:

   Return zero-based index in the list of the first item whose value is equal to *x*.
   Raises a :exc:`ValueError` if there is no such item.

   The optional arguments *start* and *end* are interpreted as in the slice
   notation and are used to limit the search to a particular subsequence of
   the list.  The returned index is computed relative to the beginning of the full
   sequence rather than the *start* argument.

I think we should update this page: https://docs.python.org/3/library/stdtypes.html#list

@picnixz picnixz marked this pull request as draft July 3, 2024 08:12
@picnixz
Copy link
Member Author

picnixz commented Jul 3, 2024

Actually, it's something that I just thought of (because I didn't run the entire test suite) but... wouldn't it actually break doctests in general...? @Eclips4

@Eclips4
Copy link
Member

Eclips4 commented Jul 3, 2024

So, there's only one test which is based on previous error message: test_xml_etree.ElementTreeTest.test_simpleops.
I think it's ok to update that one.

@picnixz
Copy link
Member Author

picnixz commented Jul 3, 2024

Actually, I was more worried about doctests in the wild that could depend on the previous error message. Instead, what I did was to keep the messages similar so that those tests would be easy to fix (minimal edit).

@picnixz picnixz marked this pull request as ready for review July 3, 2024 08:24
@Eclips4
Copy link
Member

Eclips4 commented Jul 3, 2024

Oh... we don't have an index for those methods? https://docs.python.org/3/tutorial/datastructures.html#more-on-lists

.. method:: list.index(x[, start[, end]])
   :noindex:

   Return zero-based index in the list of the first item whose value is equal to *x*.
   Raises a :exc:`ValueError` if there is no such item.

   The optional arguments *start* and *end* are interpreted as in the slice
   notation and are used to limit the search to a particular subsequence of
   the list.  The returned index is computed relative to the beginning of the full
   sequence rather than the *start* argument.

I think we should update this page: https://docs.python.org/3/library/stdtypes.html#list

I've also found out about this problem yesterday. I think we should remove :noindex: from list methods.
cc @hugovk

@picnixz
Copy link
Member Author

picnixz commented Jul 3, 2024

Actually, this part is in the tutorial so I don't know if they shouldn't be moved to stdtypes instead (https://docs.python.org/3/library/stdtypes.html#list). For instance, the string methods are out there as well.

@nineteendo
Copy link
Contributor

nineteendo commented Jul 3, 2024

Can we use the format of the error message for list.index()? It was changed from "list.index(x): x not in list" to "%r is not in list" in f6489f9. It also reduces the diff :)

@picnixz
Copy link
Member Author

picnixz commented Jul 3, 2024

Oh. Yes, I think we can since there was a precedent. I just like the fact that we known which method caused the error but maybe it's not needed?

@nineteendo
Copy link
Contributor

I don't think it's needed, that's what tracebacks are for. Also see #51501.

@picnixz
Copy link
Member Author

picnixz commented Jul 3, 2024

Mmh I see that there are a lot of previous discussions on that topic. For now, I'll convert it to a draft, pending decision on the actual format. I also think I'll need to update the deque and range objects since those may have the same issues.

@picnixz picnixz marked this pull request as draft July 3, 2024 08:45
@picnixz
Copy link
Member Author

picnixz commented Jul 3, 2024

Actually, #57558 explicitly rejected the idea of changing those messages, mainly for keeping things that worked for decades. And I saw that list/tuple are not the only ones that need to be changed. So, I think I was too hasty for that one. I agree with #57558 (comment) and so I think we should also close the issue as not planned, unless Raymond has changed its mind.

@picnixz picnixz closed this Jul 3, 2024
@xSpecialFoodx
Copy link

Actually, #57558 explicitly rejected the idea of changing those messages, mainly for keeping things that worked for decades. And I saw that list/tuple are not the only ones that need to be changed. So, I think I was too hasty for that one. I agree with #57558 (comment) and so I think we should also close the issue as not planned, unless Raymond has changed its mind.

I don't agree with his reply.
If it prints the object repr then probably the object doesn't implement the str dunder.

@picnixz picnixz deleted the index-error-message branch August 19, 2024 09:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants