Danek Duvall wrote: > On Fri, Sep 26, 2008 at 04:02:59PM -0700, Brock Pytlik wrote: > > >> http://cr.opensolaris.org/~bpytlik/ips-1651-v3/ >> > > image.py: > > - line 103: I'd just call this "ImageNotFoundException". More fully, I'd > add a boolean member to it called "specified", setting it to true when > you raise it where you do now, and replace the ValueError we're > currently raising with an ImageNotFoundException where "specified" was > False, and possibly combine the messaging around the two when caught. > Ok > - line 104: "an" -> "a" > fixed > - line 223: update the comment to reflect introduction of exact_match > fixed > - line 253-255: what's this third test for? Also, binary operators > should be at the end of a line, not the beginning. > I thought that allowing the user to either specify /foo/bar/ or (/foo/bar/var/pkg or /foo/bar/.org.opensolaris,pkg) would be more user friendly than requiring only one or the other. The second test is testing against /foo//bar and the third is testing against (/foo/bar/var/pkg or /foo/bar/.org.opensolaris,pkg). > client.py: > > - line 2110: isn't this always going to be "mydir"? Why keep this in the > exception? And I don't see you using the other two members, either. > I keep it in the exception because it removes assumptions. Specifically, only find_root knows what directory it was looking for. In this case, it's fairly obvious from the code, but in other places, it may be less obvious what find_root was called with. In general, I think exceptions should contain all the relevant information available that's reasonable to have. That's the same reason the other bits of information are being returned even though they're not currently being used (they were originally, but Dan suggested simplifying the output). If we have problems in this area, I think it will be easier to debug if we can easily get to the root dir and img_dir that find_root is choosing, which the current exception allows. (Specifically, we can ask a user to change the print line at the end of client, rather than asking them to get into the middle of image.py.) Also, another client may make a different UI choice in what to display to the user when this exception is raised. > t_R_option.py: > > - I don't think you're using "os", "time", or "stat" imports. > > Right
New webrev at: http://cr.opensolaris.org/~bpytlik/ips-1651-v4/ > Danek > _______________________________________________ pkg-discuss mailing list [email protected] http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
