Thanks for the review. > client.py > > - usage() takes an argument which is the error to print. Looks like all > of your calls to usage() with a print immediately before them should > just move that string into the usage() call.
Changed. > - line 890, 1040: spaces around equals signs. Ok. > - line 908, 913: could use error() This is inconsistently used in client.py, but okay. > - line 942, 944: these two lines are the same; take them out of the if? Which two lines? Can you either clarify or give me more context? I'm having a hard time making sense of this comment. > image.py > > - line 315: how do you remove a key (or a cert or a mirror, etc) from an > authority? Right now, you unset the authority and create a new one. You could also edit the cfg_cache by hand. What's the use case for removing a cert? > - line 567: should this be outside the if? No. If it's outside the if, you'll return None as the authority in all situations where we don't re-write the format of the installed file. I'll add a comment here, so this isn't so confusing. > - line 774: you don't reset cat each time through the loop. Is that > problematic? You might consider pushing these two lines (removing the > "if cat" test up into the previous if. Uh, yeah. I'll change that. > retrieve.py > > - line 73: "manifest", not "file" Thanks. > misc.py > > - line 41: why not "if u[-1] != '/':"? Sure. > urlhelpers.py > > - line 1: "supplement" Thanks. > - line 30: 8-space indent There were a couple of other lines with this problem too. Fixed. Thanks, -j _______________________________________________ pkg-discuss mailing list [email protected] http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
