>> http://cr.opensolaris.org/~swalker/pkg-5871/
client.py:
- line 2375: Isn't this a configuration error instead of a transport
error?
api.py:
- line 949: Insert a newline between the comment and the code on line
950
- line 960: This function never returns False
- lines 1052-1245: Do these functions belong in the API? It seems
like the ability to save/restore state should belong to the
Publisher object itself. It might make sense to refactor this using
the Memento Pattern. Wikipedia gives an example but references undo
as a primary reason for employing this technique. I don't think
that's entirely correct. One could employ this technique anywhere
state needs to be saved/restored but kept opaque to intermediaries.
http://en.wikipedia.org/wiki/Memento_pattern
api_errors.py:
- line 103: Using "permissions" a second time makes for redundant
english. What about using "privilege" instead?
image.py:
There appear to be a lot of duplicate functions here. After looking at
the changes to the API to add/remove/update/modify Publishers, aren't
the routines like set_attrs() and set_publisher() redundant? We've had
a lot of problems figuring out what functions to call when duplicates
exist in image. If at all possible, I would remove duplicate functions
from this module, and try to coalesce the code.
publisher.py:
- lines 35-41: What is this for?
- line 60: In what cases don't we want to affix a trailing slash to
the URL?
- lines 96, 110, 146: How about creating a set for schemes that can use
SSL, so we don't have to replace the value everywhere should we add
another protocol that uses SSL.
- line 132: Similar comment: If we create a set of currently supported
schemes, we don't have to update the hardcoded value in multiple
places.
- lines 241, 340, 365, 390, 401, 413, 423, 430, 440, 452, 464, 475,
490, 502, 506, 680, 685, 701, 710, 715, 719, 723, 727, 732, 743,
747, 754, 760, 768, 773, 780, 786, 792: Newline between docstring and code
The following construction shows up in a number of places. While it's
fine in its current form, it might be worthwhile to add a comment that
deletion changes the array indexes, so we must return. A less
experienced programmer might attempt to remove multiple items by
removing the return statement. This won't do what they expect, although
it _might_ work, if they're lucky.
for i, m in enumerate(self.legal_uris):
if uri == m.uri:
del self.legal_uris[i]
return
- lines 484, 499: Priority is one possible sorting for origin/mirror
preferences; however, my guess is that people would generally prefer
that the pkg client choose the mirror based upon observed network
performance. For now, I would avoid sorting these lists. It might
also be worthwhile to introduce some kind of priority policy
attribute so we can easily switch between numerical priority order,
and more complicated algorithms.
Are the deprecated methods in publisher going to be removed as part of a
subsequent putback? If these get left around, my intuition is that
they'll continue to be used as people copy other idioms elsewhere in the
code.
The rest looks fine to me. Thanks for doing this.
-j
_______________________________________________
pkg-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
