Here's version 5, which resyncs with the gate since the latest put back touches code in the same area: http://cr.opensolaris.org/~bpytlik/ips-1651-v5/
Brock Brock Pytlik wrote: > 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 > _______________________________________________ pkg-discuss mailing list [email protected] http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
