And here's part II ... On Sat, Jul 05, 2008 at 08:21:38AM -0700, Brock Pytlik wrote:
> http://cr.opensolaris.org/~bpytlik/ips-search/ search_storage.py: - line 299: loe? - line 328, et al: Changing the values to the empty string isn't deleting, exactly. Is there a reason you're doing that rather than doing "del self._list[foo]"? - line 344: I'd put the text about the code copying in a comment in the function rather than in the docstring. The latter is more to describe the external interface it presents. Also, "function calls". - line 377: "structure" - line 403: Is there a structural reason we would end up with a blank line, rather than just removing the line in the first place? - line 412, 413: given the asserts on lines 415, 416, these should be the same. Why not do something like for i, line in enumerate(file_handle): ... self._line_ct = self.next_id = i - 1 # (or whatever) - line 465, 577: use enumerate() here, too? - line 496: What's "entity number format"? - line 506: offset can't be zero? How else would either of these asserts ever trigger? You'd die with a ValueError when dereferencing res on the previous two lines, first. - line 518: I don't think this is possible -- the open() would trigger an exception first. - line 535: the entry count is always zero? - line 585: no apostrophe - line 591: blank line between methods server/server_catalog.py: - Perhaps just call this server/catalog.py - line 66 (et al): you haven't imported emsg from anywhere. The message itself appears to be garbled ("signal.signal"?), and not particularly informative. The online docs say this will happen when it's called from a thread other than the main thread. Might be worth just saying that. - line 95: delete blank line - line 96, 157: set's constructor takes an iterator: cat_fmri_set = set(set.fmris()) - line 106: It's probably worth breaking the entirety of this if clause out into another function. Or at least put figuring out where you'd find a particular program into its own function. Call it whence(). - line 132: That message probably should be cleaned up. - line 152: you're not actually following your own advice, since you're also calling it from depot.py. Of course, you have to, but you might want to rewrite the comment a bit. - line 157: there's a fair amount of duplication between this method and refresh_index(). Is there any way to reduce that? - line 178: pkg.catalog.Catalog.build_catalog() isn't used anywhere but here -- you might as well just pull that method into this class. - line 191: this message should match that on line 66, though now I see what you were trying to say there. - line 193: "not" -> "no" - line 194: "is" -> "returns" - line 201: You're calling poll() for a second time here. I assume that's safe? Regardless, I'd see if restructuring it so there's just one call wouldn't look better. - line 204: You've been using "indexes" throughout. "Indices" is the correct plural for "index", but "indexes" might be an appropriate technical plural, I dunno. Regardless, this spelling is incorrect, and it should match what you're using everywhere else. - line 219: Why don't you, then? Raising an exception here seems sketchy, especially if this is being handled in the main thread, it might bring the whole server down. - line 249: what's the point of this assignment? server_query_engine.py: - line 57, 78: why not close the file handle after this line, rather than in the finally clause? - line 69: get rid of line 69 - line 74: should this be outside the try? - line 85: get rid of blank line t_search.py: - line 237: "bug development"? - line 241: "existent" - line 330: isn't this what pkgsend_bulk() is for? - line 338: We're not doing this anywhere else in the testsuite; should we? - I may be missing it, but I don't see a test for bug 983, which would be useful to have. That's it, I think! Danek _______________________________________________ pkg-discuss mailing list [email protected] http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
