On Tue, Sep 20, 2011 at 10:05:21PM -0700, Ethan Jackson wrote:
> Please add idltest.py to the gitignore.
No longer necessary since we dropped generating it.
> @@ -373,6 +397,87 @@ class Datum(object):
> else:
> return None
>
> + 'uuid_to_row' must be a function that takes a value and an
> + ovs.db.types.BaseType and translates UUIDs into row objects."""
>
> Is there a reason to pass this function in instead of calling it
> directly from idl.py? Will people be wanting to write their own
> implementations? Does it make sense to allow a None uuid_to_row
> which defaults to the idl implementations? Same question goes for
> from_python().
I'd view calling it directly as a layering violation. A Datum doesn't
currently know anything about the IDL. Making it know directly about
ovs.db.idl.Row and what to do about it seems like a bad idea.
If you'd prefer, I'll move to_python() into the Idl code as an
internal helper, in which case no uuid_to_row argument is needed.
That's the way I had it originally, but then I thought that it was
more naturally a method on Datum.
> + @staticmethod
> + def from_python(type_, value, row_to_uuid):
>
> This function could use a comment too.
How about this:
"""Returns a new Datum with the given ovs.db.types.Type 'type_'. The
new datum's value is taken from 'value', which must take the form
described as a valid return value from Datum.to_python() for 'type'.
Each scalar value within 'value' is initally passed through
'row_to_uuid', which should convert objects that represent rows (if
any) into uuid.UUID objects and return other data unchanged.
Raises ovs.db.error.Error if 'value' is not in an appropriate form for
'type_'."""
> + The IDL uses and modifies 'schema' directly."""
> Can't 'schema' simply be cloned?
The caller is allowed to add attributes to schema columns (in
particular, 'alert') that ovs.db.schema.DbSchema won't copy.
> + def destroy(self):
> This one could use a comment. In particular, it's not obviously clear to me
> why you would call this instead of letting the GC handle it.
I think that this is redundant with Transaction.abort(), so I just
deleted it.
> + if self._comments:
> + operations.append({"op": "comment",
> + "comment": "\n".join(self._comments)})
> +
> +
> Redundant newline.
Thanks, removed.
> +def update_iface(row):
> This definition needs to be deleted.
Thanks, removed.
> def keep_table_columns(schema, table_name, column_types):
>
> This function is very similar in ovs-monitor-ipsec and ovs-xapi-sync. I think
> something like it should be pulled into the idl as a helper. I expect to be
> adding another user fairly shortly as well.
OK, would you mind doing that as part of adding the new user? This
series is long enough already.
> -def monitor_uuid_schema_cb(schema):
> +def prune_schema(schema):
> string_type = types.Type(types.BaseType(types.StringType))
> string_map_type = types.Type(types.BaseType(types.StringType),
> types.BaseType(types.StringType),
>
> I wonder if it would make sense to add a helper function to the idl for
> generating these things from python. It would take a tuple analogous to the
> init function of Type (key, value, n_min, n_max). The key or value could be
> standard python types, or another tuple in which case the type would be
> recursively generated. Does something like that sound possible/useful?
You could fairly naturally use Type.from_json() to do it, something
like this:
string_type = types.Type.from_json("string")
string_map_type = types.Type.from_json(["key": "string",
"value": "string",
"min": 0,
"max": "unlimited"])
Or we could just not bother with validating the types anymore, since
they're part of the distribution instead of retrieved remotely from
the database now.
_______________________________________________
dev mailing list
[email protected]
http://openvswitch.org/mailman/listinfo/dev
