[email protected] wrote:
On Tue, Sep 01, 2009 at 05:14:02PM -0500, Shawn Walker wrote:
- line 61 & 112: It looks like you don't remove the temporary file if
everything goes according to plan? Is this intentional, and if so,
who cleans up the tempfiles when you're done?
The idea is that the JSONWriter creates a temporary file to store the
serialized results of the data provided by the caller. Once the
serialization is complete, the caller renames the file into place
wherever they want it using json_writer_obj.pathname. Since this is a
private helper class, that seems reasonable.
Perhaps. The fact that the testsuite leaves tempfile turds everwhere
continues to be a source of aggravation. Would you comment the code to
state that you expect callers to rename this object if they want to keep
its contents? I would really like it if you also added a __del__ method
that removes the tempfile if it exits, unrenamed, when the object is
destroyed.
I've already added such a comment.
I would note that since renaming the file is a necessary part of the
operation, and this tempfile is created in the caller's specified
directory (in this case, catalog directory), not a temp directory, I
don't believe that's really necessary, but I'll still add it if you
believe so.
- line 305: Can you explain why you used super() here instead of
CatalogPartBase? All of the other code that I've seen uses the name
of the parent class explicitly.
In case there are further sub-classes in the future, this allows those
to work correctly. super() handles certain patterns of inheritance that
just simply using the base class name would not.
Would you provide a bit more detail about the situations where you
expect this to be useful? I'm curious what you're doing with this class
heirarchy that makes this (possibly) necessary.
It's not what I'm doing, but what I might do. There's no harm in using
super() vs. just calling the base class name, so I call it 'planning for
the unknown future'. I fully expect to have to subclass the catalog
code at some point, possibly for the Phase III server work; I don't know
yet.
manifest.py:
particular, it doesn't look like we take any action beyond raising
an exception if verification fails. Bug 6011 details the need to
re-download corrupt manifests. If we're able to determine that a
manifest isn't valid, the transport needs to know that it encoutered
an error while downloading the manifest, and we should re-download
the manifest's content; however, I don't see either of these things
happening here. Is this part of Phase III?
Yes and no. For the client to get the manifest signature data, the
server has to provide v1 catalogs. So the completion of the plumbing
work will be done in Phase III, but I will be opening a bug for another
enterprising soul to take advantage of the new aqueduct :)
Yes, however the step that catches either a MalformedActionError or a
MyManifestDidntVerify error ought to be generic. I would suggest that
since we're going to the trouble of adding manifest signatures and
catalog signatures, we should verify them both too. I'm willing to do
the transport side of the work for both of these, but I think we need to
be in agreement that this is part of the Phase III deliverables in order
for that to happen.
My current schedule demands don't allow me to personally be the party to
deliver those, so I'll have to pass. I'm putting all the plumbing in
place because it is so inter-twined with the existing work I'm doing,
but I really don't have the time to do the extra work on top of that.
Cheers,
--
Shawn Walker
_______________________________________________
pkg-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
