GNOME Bugzilla – Bug 639939
GVariant creator is not recursive and does not support empty structures
Last modified: 2011-01-20 16:59:53 UTC
The current GVariant creator currently only handles very simple cases, i. e. "leaf" types and nonempty tuples/arrays/dicts: Does not accept empty tuples, lists, dictionaries, as it does not properly parse the data type of the elements: $ python -c "from gi.repository import GLib; print GLib.Variant('()')" IndexError: tuple index out of range $ python -c "from gi.repository import GLib; print GLib.Variant('ai', [])" gi/types.py:40: Warning: g_variant_builder_end: assertion `!GVSB(builder)->uniform_item_types || GVSB(builder)->prev_item_type != NULL || g_variant_type_is_definite (GVSB(builder)->type)' failed return info.invoke(*args) None $ python -c "from gi.repository import GLib; print GLib.Variant('a{ii}', {})" $ python -c "from gi.repository import GLib; print GLib.Variant('a{ii}', {})" gi/types.py:40: Warning: g_variant_builder_end: assertion `!GVSB(builder)->uniform_item_types || GVSB(builder)->prev_item_type != NULL || g_variant_type_is_definite (GVSB(builder)->type)' failed return info.invoke(*args) None Does not accept actual tuples for tuple inputs, as it always assumes the arguments for the tuple to be in the "flat" argument list: $ python -c "from gi.repository import GLib; print GLib.Variant('(ss)', ('mec', 'mac'))" TypeError: argument 1: Must be string, not tuple Does not accept nested dicts, as the function doesn't recurse properly: $ python -c "from gi.repository import GLib; print GLib.Variant('a{sa{si}}', {'a': {'b': 1}})" IndexError: string index out of range For using GVariants properly in Python, especially with GDBus and trying to call more complex D-Bus methods.
I am currently working on a better implementation of it, and have added tons of test cases. All the stuff works now, I just want to make the code a little cleaner. Due to all the new cases it became a tad more complex, but not actually longer (the old code was not exactly easy to understand either..).
Created attachment 178719 [details] [review] git formatted patch This now passes all the test cases. The code is about the same size, but fully recursive now, and handles empty structures and real tuples.
For the record, neither the current nor this proposed patch supports the 'v' type (gvariants) yet. I'll add that as well, but did not want to make the first patch even bigger.
Created attachment 178818 [details] [review] git formatted patch Updated patch with some test cases for error conditions and some whitespace fixes.
Review of attachment 178818 [details] [review]: Excellent patch! Have raised just some style nitpicks, please push once you have addressed them. ::: gi/overrides/GLib.py @@ +45,3 @@ } + def create(self, fmt, args): s/fmt/format for easier reading. Please check for other unnecessary abbreviations in the rest of the patch (lc, v, ...). @@ +77,3 @@ + return self.create_array(fmt, args) + + raise NotImplementedError, 'cannot handle GVariant type ' + fmt This will break in Python 3.x, better use NotImplementedError() @@ +79,3 @@ + raise NotImplementedError, 'cannot handle GVariant type ' + fmt + + def create_tuple(self, fmt, args): Does this and the other create_* methods need to be public? @@ +105,3 @@ + + builder = GLib.VariantBuilder() + if args is None or len(args[0]) == 0: See PEP-08 about using "not args[0]" instead of "len(args[0]) == 0". Check for other instances of this in the rest of the patch. @@ +111,3 @@ + rest_format = self.create(rest_format, None)[1] + if not rest_format.startswith('}'): + raise TypeError, 'dictionary type string not closed with }' Not ValueError? @@ +113,3 @@ + raise TypeError, 'dictionary type string not closed with }' + rest_format = rest_format[1:] # eat the } + element_type = fmt[:len(fmt)-len(rest_format)] spaces around operators @@ +158,3 @@ + creator = _VariantCreator() + (v, restfmt, restargs) = creator.create(format_string, list(args)) + if restfmt != '': if rest_format: (pep-08) ::: tests/test_overrides.py @@ +178,3 @@ + def test_gvariant_create_errors(self): + # excess arguments + self.assertRaises(StandardError, GLib.Variant, 'i', 42, 3) Maybe use TypeError to be consistent with the rest of PyGObject?
Thanks Tomeu for the review! I updated the patch according to your remarks and pushed it.