bpo-13349: Fix error reporting for index and remove methods

[DimitrisJim]

Fixes error reporting for the index and remove methods in list, tuple, deque, operator.indexOf, element and in abstract.c. Also fixes an occurence of this message in the doctest documentation.

Instead of repeating the method name as in the original patch supplied by Sean.Ochoa on b.p.o, it changes all occurences of:

object.method(repr(x)): repr(x) not in object

to the more concice:

repr(x) not in object

the repr is trimmed to 100 characters as suggested on b.p.o.

Added necessary tests to check for the presence of the value in the exception.

Specifically, for .index:

• test_index in seq_tests runs for: list.index, tuple.index and deque.index
• test_index in test_array for: array.index
• test_indexOf in test_operator for: operator.indexOf

While, for .remove:

• test_remove in list_tests for: list.remove
• test_remove in test_deque for: deque.remove
• test_remove in test_array for: array.remove
• test_simpleops in test_xml_etree for: element.remove

---

The only open question I currently have is:

1. In each of these test cases, should I add an additional check that asserts that the value in the exception message is trimmed?

[mention-bot]

@DimitrisJim, thanks for your PR! By analyzing the history of the files in this pull request, we identified @rhettinger, @serhiy-storchaka and @eliben to be potential reviewers.

[SylvainDe]

Interesting issue!
What about PyErr_SetString(PyExc_ValueError, "sequence.index(x): x not in sequence"); (in abstract.c) ; PyErr_Format(PyExc_ValueError, "%R is not in range", ob); (only the truncation is missing) (in rangeobject.c) ?

[DimitrisJim]

@SylvainDe About abstract.c: From what I know, that isn't tested anywhere, I might be wrong but a core-dev would need to clarify that.
About the truncation in rangeobject.c: there's probably quite a number of those around but truncating all of them was beyond the scope of the issue. I merely unified all error messages on .index and .remove I could find; no need to fix unrelated issues and make the code-review harder for people :-)

[SylvainDe]

@DimitrisJim Sure ! Thanks for the answer :-)

[rhettinger]

I'm not sure this is a good idea. Do we have any evidence that the current error messages are causing problems for users. At least some of this code is well over a decade old and I don't recall a single user complaint. In addition, I don't think we necessarily want to make %.100R the new norm except in cases where the original author thought is would be helpful.

[DimitrisJim]

Hi @rhettinger! Personally, I haven't seen any users facing problems due to the specific error message. This is probably because it is generally easy to use print before the offending call to get information on the value used or it might be included in the traceback if a literal was used.

On the other hand, it's a shame to have the information on what value was attempted to be removed/looked-up with remove and index readily available and not give it to the user. Doing so makes debugging errors easier and thankfully does it with very minor and non-intrusive changes to the code. This and the fact that no dissenting opinions existed on b.p.o made be think it might be a good change.

As for %.100R I'd agree and I'd be more than willing to roll-back the change to deque where I only made a change from %R to %.100R if, as the author, you think it might be too defensive and not helpful.