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

Elements reports it is a list on Element.remove #74692

Closed
DimitrisJim mannequin opened this issue May 29, 2017 · 3 comments
Closed

Elements reports it is a list on Element.remove #74692

DimitrisJim mannequin opened this issue May 29, 2017 · 3 comments
Labels
3.7 (EOL) end of life stdlib Python modules in the Lib dir type-bug An unexpected behavior, bug, or error

Comments

@DimitrisJim
Copy link
Mannequin

DimitrisJim mannequin commented May 29, 2017

BPO 30507
Nosy @rhettinger, @scoder, @DimitrisJim

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-31.14:04:02.434>
created_at = <Date 2017-05-29.14:49:29.621>
labels = ['3.7', 'type-bug', 'library']
title = 'Elements reports it is a list on Element.remove'
updated_at = <Date 2017-05-31.14:04:02.433>
user = 'https://github.com/DimitrisJim'

bugs.python.org fields:

activity = <Date 2017-05-31.14:04:02.433>
actor = 'Jim Fasarakis-Hilliard'
assignee = 'none'
closed = True
closed_date = <Date 2017-05-31.14:04:02.434>
closer = 'Jim Fasarakis-Hilliard'
components = ['Library (Lib)']
creation = <Date 2017-05-29.14:49:29.621>
creator = 'Jim Fasarakis-Hilliard'
dependencies = []
files = []
hgrepos = []
issue_num = 30507
keywords = []
message_count = 3.0
messages = ['294697', '294704', '294844']
nosy_count = 4.0
nosy_names = ['rhettinger', 'scoder', 'eli.bendersky', 'Jim Fasarakis-Hilliard']
pr_nums = []
priority = 'normal'
resolution = 'rejected'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'behavior'
url = 'https://bugs.python.org/issue30507'
versions = ['Python 3.7']

@DimitrisJim
Copy link
Mannequin Author

DimitrisJim mannequin commented May 29, 2017

Another splinter issue from bpo-13349. Currently, Element reports it's a list when remove is called on it:

    from xml.etree.ElementTree import Element
    Element('').remove(Element('')) 
ValueError: list.remove(x): x not in list

From what I understand, this was done in order for it to conform with the error reporting performed from the pure python implementation of Element. (side note: These also differ regarding the type of value supplied to .remove, the C implementation only wants instances of Element)

The message, imo, is confusing and should be changed to Element.remove(x): x not in Element.

@DimitrisJim DimitrisJim mannequin added 3.7 (EOL) end of life stdlib Python modules in the Lib dir type-bug An unexpected behavior, bug, or error labels May 29, 2017
@rhettinger
Copy link
Contributor

The message, imo, is confusing

Sorry, I think this is an invented issue and not a actual problem. ElementTree has been in the wild for a very long time. AFAICT, no user has ever had an actual issue with this error message. I've taught ElementTree (specifically including this method) at least one per month over six years and have never seen the slightest issue with these error messages (in PyPy, the actual ElementTree.py is used).

It would just be code clutter to wrap this and all the other accessors in:

try:
     self._children.remove(subelement)
except ValueError:
     raise ValueError('Element.remove(x): x not in Element')

I would like the standard library to *not* be filled with this sort of thing. The norm in pure Python code is to let the error checking occur at the lowest level and propagate its way up with its original error message. Usually, the only time we rewrap the exception is if the message would be far off from the actual cause.

I'm concerned at the recent rise of "invented issues" where newcomers posit that long deployed, stable code, written by very smart people must be changed even though no actual user has ever been encountered.

Invented issues waste developer time and distract from the numerous real issues we have to deal with. It also seems to be encouraging a mentality of roaming through the codebase with a desire to make numerous trivial changes and while second-guessing all the design decisions made a long time ago. This isn't good for our project.

Guido has long encouraged "holistic refactoring" for a reason. This particular tracker issue is a case study about why that is the case. The origin of this issue was grepping the standard lib for "ValueError" and "remove" with the aim of second guessing every message. It was not born out of actual usage of or concern for the ElementTree module.

The reason that matters is that someone working with the module as a whole would have realized that this tracker issue is at odds with its creator's intended design as a thin wrapper around lists. The docs explicitly describe the API "as a cross between a list and a dictionary".

This one error message isn't atypical (you get the same mention of "list" in most of the methods such as __getitem__, __setitem__, __delitem__, insert, etc).

We really don't want to start a practice of having every pure python class that accesses a list or dict revise its code to rewrap the error messages just to change the class mentioned in the message. This isn't a normal pythonic practice. It would just clutter, constipate, and slow down the code for nearly zero benefit.

We really should place more value on code stability, start focusing on modules holistically, respond to actual user issues rather than invented issues, avoid frequent trivial changes, and consider that the developers who came before us might have given a great deal of thought into their code.

@DimitrisJim
Copy link
Mannequin Author

DimitrisJim mannequin commented May 31, 2017

Thanks for the feedback, Raymond. I'll try to shift my focus on more pressing issues. (Closing as rejected)

@DimitrisJim DimitrisJim mannequin closed this as completed May 31, 2017
@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
Labels
3.7 (EOL) end of life stdlib Python modules in the Lib dir type-bug An unexpected behavior, bug, or error
Projects
None yet
Development

No branches or pull requests

1 participant