attrib.pop for comments/PI broken

[Kovid Goyal]

attrib.pop('xxx', None) raises AttributeError: 'dictproxy' object has no attribute 'pop' for comments/PIs/ This used to work in older lxml versions, and it breaks commonly used code like

[x.attrib.pop('style', None) for x in root.iter()]

to remove an attrib from all elements.

I strongly suggest implementing a dummy pop method for comments/PIs or updating lxml will break a *lot* of code.

Test case:

python -c 'from lxml.etree import fromstring; r = fromstring("<r><!--x--></r>"); [x.attrib.pop("xxx", None) for x in r.iter()]'

Version info:
>>> import sys
>>> from lxml import etree
>>>
>>> print("%-20s: %s" % ('Python', sys.version_info))
Python : sys.version_info(major=2, minor=7, micro=6, releaselevel='final', serial=0)
>>> print("%-20s: %s" % ('lxml.etree', etree.LXML_VERSION))
lxml.etree : (3, 3, 0, 0)
>>> print("%-20s: %s" % ('libxml used', etree.LIBXML_VERSION))
libxml used : (2, 9, 1)
>>> print("%-20s: %s" % ('libxml compiled', etree.LIBXML_COMPILED_VERSION))
libxml compiled : (2, 9, 1)
>>> print("%-20s: %s" % ('libxslt used', etree.LIBXSLT_VERSION))
libxslt used : (1, 1, 28)
>>> print("%-20s: %s" % ('libxslt compiled', etree.LIBXSLT_COMPILED_VERSION))
libxslt compiled : (1, 1, 28)

[scoder (scoder)]

> ... or updating lxml will break a *lot* of code.

Well, it's certainly the first time I see this use case, so I may raise my doubts that it's all that common. But yes, it's a regression, so it should be fixed.

[Kovid Goyal (kovid)]

I use it all the time in calibre, and calibre has been installed ~ 20 million times.

[scoder (scoder)]

any reason you're not using .iter('*') to iterate only over Elements ?

[Kovid Goyal (kovid)]

No particular reason, I sometimes use iter(etree.Element) and sometimes use iter(). The latter is when I am lazy/forgetful, or when the code is not written by me.

The point is that using iter() is the default usage. Most people wil forget about comments/PIs when writing code. Therefore, breaking it is likely to break a lot of code.

[scoder (scoder)]

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.

[scoder (scoder)]

Actually, this is only a problem with Comments, not PIs (which have a sort of pseudo attribute emulation).

[scoder (scoder)]

Implemented here:

https://github.com/lxml/lxml/commit/17928f9e048c549c140ade0ebf0e4ae749054d22

[Kovid Goyal (kovid)]

Thanks for the fix :)

[scoder (scoder)]

Fixed in lxml 3.3.2.

[Attila Lendvai (attila-lendvai)]

FTR, this bit me also while using ebook-convert. not urgent, so just waiting for the fix to reach debian...