Looks fine to me. Brock Pytlik wrote: > 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
-- Shawn Walker _______________________________________________ pkg-discuss mailing list [email protected] http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
