Code review comment for lp:~jml/testtools/more-content-convenience

Will look at the code todayish.

On Fri, Dec 31, 2010 at 3:38 AM, Jonathan Lange <email address hidden> wrote:
>  * I've dropped attachFile. We can add it later if the people clamor for it.
>
>  * I don't think they'd be better as stand-alone functions.  At the very least, standalone functions can be implemented in terms of classmethods (e.g. content_from_file = Content.from_file), but classmethods can only be built from functions if they drop the class-based dispatch.  As far as I can tell, it's a matter of taste.  I am willing to budge on this one.

I think doing 'TracebackContent.from_file' would be hugely confusing.
I'll see about whipping up a comparitive patch.

>  * I've added documentation to from_stream saying that it will close the stream when the iterator is exhausted, and made it actually close the stream.  The reasoning is that testtools doesn't really offer any sort of hook for a sensible place to close the stream.

>  * I've changed the read_now path in from_file to be completely separate from the lazy read path.
>
>  * I couldn't get the fixture thing to work.

What didn't work?

> Unfortunately, without attachFile, we still don't have a nice syntax for the common case of "add this file as a detail during teardown".  The best we have is:
>  self.addCleanup(lambda: self.addDetail('foo', Content.from_file('foo.txt', read_now=True)))
>
> As opposed to:
>  self.addCleanup(self.attachFile, 'foo', 'foo.txt', read_now=True)

I see, the lambda is needed because we want to force Content.from_file
to execute at cleanup time, not before.

And what we're really saying is 'try to read this thing during
cleanup, and cache the result cause its going to get deleted'.

So there are a few related bits here; the first is that the case I
expect this to happen in, is the case where the file being read is a
log file for a temporary server. I'd expect that server to have a
fixture, and the fixtures /details/ will be automatically captured
during cleanup. So the place that 'attachFile during cleanup' as a
convenience method is most needed is in fixtures. or somewhere where
it is usable by both TestCase and Fixture.

def attach_file(path, obj_with_details, name=None, lazy_read=False):
     """Attach the contents of the file at path to obj_with_details.

     This function adds a 'details' object to obj_with_details which will
     return the content of path. By default the content is read immediately
     and cached.

    :param path: The path of the file.
    :param obj_with_details: The object to attach the file to -
generally a TestCase or Fixture.
    :param name: Optional name for the file. If not specified the
basename is used.
    :param lazy_read: If True the file content is not read when
attach_file is called, but later
        when the content object is evaluated. Note that this may be
after any cleanups that
        obj_with_details has, so if the file is a temporary file
lazy_read may cause the file to
        be read after it is deleted. To handle those cases, using
attach_file as a cleanup is
        recommended::
             obj_with_details.addCleanUp(attach_file, 'foo.txt',
obj_with_details)
    :return: None
    """