Comment 5 for bug 1281139

Revision history for this message
scoder (scoder) wrote :

The reason why I made the "attrib" value of PIs/Comments read-only is that I consider it better to raise an error on modifications when the underlying object doesn't support them, than to silently accept these modifications and just drop them when the mapping object gets discarded. So this change was really intentional.

I find it reasonable for Comment/PI to have an immutable mapping as their "attrib" value. The fact that "attrib.pop(x, default)" doesn't actually modify the (empty) mapping is rather a coincidence. As you can see from the Python docs, .pop() is not an API method of immutable mappings but only of mutable ones:

http://docs.python.org/3/library/collections.abc.html#module-collections.abc

That being said, I already agreed that this is an unfortunate regression. It breaks code, so there should at least have been a deprecation phase. But the mere fact that there *is* an "attrib" proprety on Comments at all shows that the intention is to keep their interface compatible with that of Elements.

I also take your point that it's easy to forget about non-Elements in the tree. ElementTree's parser even discards them by default, and I might convince myself at some point that that's the better default behaviour, because it requires users to be explicit about non-Elements in the tree. I guess that behaviour won't change in lxml, though.

So, I think the only reasonable way to move on with this is to implement a new mapping type in lxml that has the complete interface of MutableMapping but raises TypeError on modifications. Shouldn't be too hard, given that DictMixin and MutableMapping are just a conditional import away.