[ovs-dev] [python idl 12/16] ovs.ovsuuid: Get rid of ovs.ovsuuid.UUID class.

[Ben Pfaff]

On Tue, Sep 20, 2011 at 04:55:36PM -0700, Reid Price wrote:
> >             for uuid_string, row_update in table_update.iteritems():
> > -                if not ovs.ovsuuid.UUID.is_valid_string(uuid_string):
> > +                if not ovs.ovsuuid.is_valid_string(uuid_string):
> >                     raise error.Error('<table-update> for table "%s" '
> >                                       'contains bad UUID "%s" as member '
> >                                       'name' % (table_name, uuid_string),
> >                                       table_update)
> > -                uuid = ovs.ovsuuid.UUID.from_string(uuid_string)
> > +                uuid = ovs.ovsuuid.from_string(uuid_string)
> >
> 
> I'm getting enough deja vu to know I've said this before =/
> If it wasn't for your dastardly-ly informative error messages, this would be
> better-written as
> 
>   uuid = ovs.<...>.from_string(uuid_string)
> 
> and letting that function do the raising.  Fine as-is, of course.

I do like having as much context as I can get in error messages,
within reason.  It makes systems easier to debug.  So I think I'll
stick with it for now.

> > +uuidRE = re.compile("^(%s{8})-(%s{4})-(%s{4})-(%s{4})-(%s{4})(%s{8})$"
> > +                    % (("[0-9a-fA-f]",) * 6))
> >
> 
> Typo, second F should be capitalized.  

Thank you!

> Could also use the dictionary-style
> string formatting, though this seems reasonable as-is.
> 
>   "^(%(hex)s{8})-(%(hex)s{4})- ... " % {'hex': "[0-9a-fA-F]"}

I agree that that is prettier, but it would force the regex to be
split between lines, which I don't like.

Hmm, this version is more readable:
    uuidRE = re.compile("^xxxxxxxx-xxxx-xxxx-xxxx-xxxxxxxxxxxx$"
			.replace('x', '[0-9a-fA-F]'))
I think I'll go with that.

> I have a preference for the OO organizational style [I think u.is_zero()
> makes more sense than ovs.ovsuuid.is_zero(u)], but I agree that these
> changes are better than having a half-classed version that is mixing and
> matching elsewhere.  

I agree that the OO organizational style is better.  My real problem
with having a class is that you end up with instances of it some
places and not others.  The Python IDL code does an unfortunate amount
of "if type(foo) in (...)" logic, which means that we have to check
for both classes.

I wish there was a simple way to just test for "iterable" and
"mapping" types (is there?).  Then we could get rid of a lot of ugly
tests for specific types, in favor of more general tests.

> If the long string were the problem, you can always do
> "UUID = ovs.ovsuuid.UUID" up at the top.

No, I don't care much about the length.

> Wanted to note that in general the errors provided by UUID itself are fairly
> informative, though they lack the object, e.g.:

Yeah, I realize that the errors that the OVS python code works to
generate are pretty unnecessary from some valid points of view.  The
main reason that it goes to this trouble is that it allows the unit
tests to check for exactly worded error messages produced in negative
tests (without having to worry that different Python versions might
have different messages), and furthermore to allow using exactly the
same tests for both the Python and C IDLs.

> > +    hex = uuid_.hex
> > +    return ["%s.parts[%d] = 0x%s;" % (var, x, hex[x * 8:(x + 1) * 8])
> > +            for x in range(4)]
> 
> hex is a (rather convenient) builtin you might want to avoid shadowing, and
> might even be able to use here (or just "0x%x"), didn't look beyond patch
> context though.

I changed 'hex' to 'hex_string' to avoid shadowing.

This generates code that looks like:
	var.parts[0] = 0x12345678;
	var.parts[1] = 0x9abcdef0;
	var.parts[2] = 0xaaaa5555;
	var.parts[3] = 0x11112222;
I don't think we can use anything simpler here.

Thank you for all the comments!