[qmtest] [PATCH] Run metadata and first pass at SQL results storage.

[Mark Mitchell]

On Wed, 2003-07-02 at 19:53, Nathaniel Smith wrote:
> Attached for review.  This patch also renames 'ResultSource' to
> 'ResultReader', since that better describes the interface we ended up
> with.

Nathaniel --

This patch is very good.  I'm a little pedantic (as you'll see below),
but the bottom line is that this should go in, after a few very minor
changes get made.  Nice work!

-- Mark

+	* qm/extension.py (get_class_arguments): Mark's fixes to
+	support diamond inheritance.

This is too cryptic.  If you really want to give me credit, put my
name in the ChangeLog entry.  But there's no need for that -- just say
"Support diamond inheritance."

+    'time_secs' -- the time to be formatted, as returned by
+                   e.g. 'time.time()'."""

We don't indent the second line: just write:

   'time_secs' -- the time to be formatted, as returned by
   e.g. 'time.time()'."""

+class Connection:
+    """A little wrapper around a DB 2.0 connection that preserves a
+    reference to the containing module, and provides a minimal
+    interface to said connection.  This is useful because it gives us
+    a hook to attach our SQL-quoting code in to."""

Every class or method should have a 1-line description and then some
details, like so:

   """A wrapper around a DB 2.0 connection.

   An instance of 'Connection' preserves a reference to the containing
   module, and provides a minimal interface to said connection.  This
   is useful because it gives us a hook to attach our SQL-quoting code
   in to."""

But then, showing massive amounts of uptightness, we'd fix this
because (a) we don't use "this" as a noun, only as an ajdective, and
(b) we avoid the first person in comments.  Unfortunately, these
commandments have not always been obeyed. :-(  But, we want something
like the following for the second sentence:

  In addition, a 'Connection' quotes SQL queries as necessary for the
  underlying DB 2.0 connection.

+def connect(modname, *args, **moreargs):

Why not make this the __init__ method for Connection?

+                        ["dummy element to make __import__ behave"])

Wasteful: should just be [""] with a comment:

  # There must be at least one element or __import__ will crash.

+def quotestr(string):
+    """Quotes a string for SQL."""
+
+    # Replace each ' with '', then surround with more 's.  Also double
+    # backslashes.  It'd be nice to handle things like quoting
non-ASCII
+    # characters (by replacing them with octal escapes), but we don't.
+    return "'" + string.replace("'", "''").replace("\\", "\\\\") + "'"

Two comments:

(1) Does it make sense to make this a member of Connection?

(2) The:

    'string' -- ???

    returns -- ???

    comments are missing.

+# A nice subtlety is that because of how extension classes are loaded,

Let's not be editorial: it's a subtlety, but not a nice one.

+_annotation_sentinel = None

Shouldn't these be packaged up into a mix-in for
PickleResult{Stream,Reader}?

+    "Version 1", and all later versions, contain a pickled version
+    number as the first thing in the file.  In version 1, this is

Thanks for spelling this out!

+        elif version == 1:
+            self._ReadMetadata()

I'm not sure why we read this at __init__ time rather than lazily when
GetAnnotations is called, but that can be changed easily later if it
needs to be.

+class SQLConnected(Extension):
+    """Mixin class for classes that need a database connection."""

Shouldn't this be a module-private class? (e.g., _SQL_Connected?)

+        for result_id, result_kind, key, value in self._a_buffer:
+            if (result_id, result_kind) != (id, kind):
+                self._a_buffer.rewind()
+                break

Interesting -- I see that you avoid a separate SELECT per result.

+class XMLResultReader(FileResultReader):

Should this go in xml_result_stream.py?  (It seems to make sense to
group the readers and writers together?)

+                rs.WriteAnnotation("qmtest.run.end_time", end_time_str)

That's a slight extension of the "foo.bar" format we've used until
now; we've never had two "." characters in an annotation.  I think
that's OK, though; it's a natural generalization and nothing depends
on that.

     A 'StorageResultsStream' does not write any output.  It simply
     stores the results for future display."""

-    def __init__(self):
+    def __init__(self, arguments):

What's the rationale for this change?

+        # Put in a note.  No software currently pays any attention to
+        # this key, but it's useful to mark runs that were done from
+        # the GUI, because they may be an amalgamation of multiple
+        # runs, and therefore cannot be trusted to describe a single
+        # version of the software under test.
+        self.__annotations = { "qmtest.created_from_gui": "true" }

Let's leave this out for now.  It's not that I disagree, but I'm also
not sure that I agree, and we might want some kind of more general
"where did this run come from?" field.