On Wed, Sep 21, 2011 at 10:58 AM, Ben Pfaff <[email protected]> wrote:
> 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.
>
Shockingly readable, I like 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.
>
The standard way to determine iterables is with
hasattr(obj, '__iter__')
It's not the prettiest (and note behavior with strings/unicode), but it
generally behaves as expected.
There isn't a great way to determine whether something is a 'map' in
general, as {}[key] and [][index]
both use the __getitem__ method. In practice, it is fairly rare to see
non-dict maps.
>>> def iterable(obj):
... return hasattr(obj, '__iter__')
...
>>> for v in ('', 1, AssertionError, [], {}, set(), xrange(1)):
... print "%40r - %s" % (v, iterable(v))
...
'' - False
1 - False
<type 'exceptions.AssertionError'> - False
[] - True
{} - True
set([]) - True
xrange(1) - True
> > 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!
>
Not at all, thanks for the responses
-Reid
_______________________________________________
dev mailing list
[email protected]
http://openvswitch.org/mailman/listinfo/dev
