Danek Duvall wrote: > On Mon, Jan 12, 2009 at 05:39:25PM -0600, Shawn Walker wrote:
Items removed have been addressed. >> http://cr.opensolaris.org/~swalker/pkg-5999-6010-2/ > > version.py: > - line 145: isn't the "*" in self[-1] case already covered? If self is > "3.2.4" and other is "3.2.4.3", then ls == 3, and we trigger the > exception on i == 3, but we already know that self[-1] == self[2] != > "*" because we looked at it on the previous iteration of the loop (when > i == 2). There's intentional trickery afoot; see below. > I would also argue that "3.2.4" doesn't match "3.2.4.*", which I think > this implies. Actually, it's intentional that 3.2.4 matches 3.2.4.*, since I'm fairly certain (based on questions I've asked others in #pkg-discuss and my own understanding), that users will expect "pkg list [email protected].*" to list all 2.2 versions of apache, including just "2.2". > - line 531: caching the string representation of the version really was > that much of a speedup? This isn't for a speedup; it is the only place to capture what I believed to be the "original intent" of the user. In other words, the user may specify one thing (e.g. 2.*), and I may end up transforming that to something completely different internally for matching purposes. > - get_short_version(): Why is this copied from Version? With the > exception of the test, it's identical, and if you need a specific test > against None (do you?), couldn't that just move into Version? Because MatchingVersion does not inherit from Version because not all of the methods on Version are applicable to MatchingVersion. My thought was that it was better to not offer methods that aren't supported on MatchingVersion instead of raising exceptions when called on a MatchingVersion object. What should I do? > - ditto set_timestamp() See get_short_version() comment. > - get_timestamp(): If you reorder the tests, you could reuse > super.get_timestamp(). See get_short_version() comment. > - __lt__(), __gt__(): I guess this won't happen much, but if you compare > two MatchingVersion objects that have asterisks for, say, release > values, self will always compare less than other, even if it's got a > greater, say, branch value. This may not be worth fixing. I've changed this slightly, I hope this is better now... > - __cmp__() looks identical. See get_short_version() comment. Updated webrev: -- Shawn Walker _______________________________________________ pkg-discuss mailing list [email protected] http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
