Code review comment for lp://staging/~free.ekanayaka/storm/schema-uri-in-testresource

Revision history for this message
Björn Tillenius (bjornt) wrote :

This looks reasonable to me. Since I'm no Storm expert, I do have some questions, though?

[1]

+ @param databases: A C{list} of C{dict}s holding the following keys:
+ - 'name', the name of the store to be registered.
+ - 'uri', the database URI to use to create the store.
+ - 'schema', the L{Schema} for the tables in the store.
+ - 'schema-uri', optionally an alternate URI to use for applying the
+ schema, if not given it defaults to 'uri'.

Maybe it would be worth adding a note about the old deprecated format as well? It will save someone some confusion, when he see that the code he has doesn't match the documentation.

[2]
+ schema_zstorm = ZStorm()
+ databases = self._databases
+
+ # Adapt the old databases format to the new one, for backward
+ # compatibility. This should be eventually dropped.
+ if isinstance(databases, dict):
+ databases = [{"name": name, "uri": uri, "schema": schema}
+ for name, (uri, schema) in databases.iteritems()]
+
+ for database in databases:
+ name = database["name"]
+ uri = database["uri"]
+ schema = database["schema"]
+ schema_uri = database.get("schema-uri", uri)
+ self._schemas[name] = schema
                 zstorm.set_default_uri(name, uri)
+ schema_zstorm.set_default_uri(name, schema_uri)
                 store = zstorm.get(name)
                 self._set_commit_proxy(store)
- schema.upgrade(store)
+ schema_store = schema_zstorm.get(name)
+ schema.upgrade(schema_store)

I guess we will always have two connections to the database, even if the same user is to be used? Will this have the risk of causing any trouble for someone? Would it make sense to set schema_zstorm = zstorm, if no schema_uri is provided, to have it work exactly as before, if no specific user is specified.

[3]

Specifying a schema URI requires you to repeat certain things from the normal URI, like database type, port number, host name, database name. Would it be possible to come up with an API where you specify only a username and password?

review: Needs Information

« Back to merge proposal