[MERGE] Implement xml8 serializer
ian.clatworthy at internode.on.net
Wed Apr 2 07:47:30 BST 2008
Aaron Bentley wrote:
> Hi all,
> This patch implements the xml8 serializer which will be used by the
> upcoming pack-1.4 format.
Thanks for working on this. I'm not 100% sure I know enough about this
area to give the only review but the code seems straight-forward and I'm
keen to see this land in 1.4.
This needs a NEW entry. Also, xml8._check_revisions() needs its
docstring updated: "By default no checking is done." is no longer correct.
I also like how you replaced some of the asserts with explicit checks, e.g.
> - assert elt.tag == 'inventory'
> + if elt.tag != 'inventory':
> + raise errors.UnexpectedInventoryFormat('Root tag is %r' % elt.tag)
It would be nice to do this in xml5._unpack_inventory() as well.
> This patch changes the inheritance so that older formats inherit from
> new ones. This means that the current format is all in one place, and
> we can drop support for old formats easily. It also makes it easy for
> future formats to use the same numbers for their revision and inventory
I'm good with this approach. The one thing that stands out though as a
consequence is that a bunch of "general" escaping functions have moved
from xml5.yyy to xml8.yyy. If and when we were to drop support for xml8
in N years time, we'd arguably be caught doing the same shuffle again.
That's a long way of saying that they arguably have nothing to do with
any particular version and ought to be moved to a more generic module,
e.g. xml_serialiser or xmlescape, say. I'm not saying that's required to
land this, though it does seem as good a time as any to put them in the
"right" place given we're moving them as part of this patch.
Finally, I did a code review of xml_serializer in order to gain enough
context to review this. This bit is either wrong or confusing:
> def read_inventory(self, f, revision_id=None):
> return self._unpack_inventory(self._read_element(f),
> except ParseError, e:
> raise errors.UnexpectedInventoryFormat(e)
The call to _unpack_inventory always passes None. If this is deliberate,
it needs a comment. My guess is that it's a bug. Could you please let me
know and perhaps address it in your patch? I'm happy to put through a
patch if you prefer once I know the right answer.
More information about the bazaar