> > http://cr.opensolaris.org/~johansen/webrev-cat-inc/index.html
>
> All nits -- I'm much happier with this than before.
Thanks for the feedback, it's much appreciated.
> catalog.py:
>
> - line 553: this could be simplified to
>
> return self.attrs.get("Last-Modified", None)
Changed.
> updatelog.py:
>
> - line 59: Could we have a comment explaining the 336?
Yeah, absolutely. It's 336 hours of history or 14 days. It's
adjustable since I have no idea how much history production systems
would actually want to keep.
> - line 158: I think this can be written as self._recv_updates(), if you
> find that clearer.
I'm not sure if I find that clearer. Do you have a preference here?
> - line 275: here's another place you can get rid of "default = ".
Removed. In fact, I could get rid of the None too, since None is the
default return value if the header doesn't exist.
> - line 333: does this need to be continued like it is?
No. I don't know why it was like that. I've put it on the same line as
the rest of the conditional. Thanks for catching that.
> - line 401: I know you did this for Catalog as well, but is there a
> reason that you did this little alias, rather than just defined the
> function outside the class?
I was inclined to keep this method in the class since it has specific
knowledge about things in the UpdateLog, and would seem to belong there.
Stylistically speaking, you seem to be annoyed by these types of
aliases, and by the import syntax that would allow me just to import
UpdateLog instead of the entire updatelog module. (from updatelog import
UpdateLog)
I'm annoyed by taking methods that belong with a class and putting them
elsewhere. I also find it annoying to have to include the name of the
calling module when I'm calling a static method that belongs to a class.
I would much prefer UpdateLog.recv() to updatelog.recv(); however, our
style guidelines have evolved based upon objections and don't seem to be
completely codified.
I'm open to a compromise. Is there a mutually pleasing alternative?
-j
_______________________________________________
pkg-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
