[email protected] wrote:
http://cr.opensolaris.org/~swalker/pkg-5871/
client.py:
All items resolved have been removed from this reply.
- line 2375: Isn't this a configuration error instead of a transport
error?
The old code use RESULT_FAILED_TRANSPORT, but I've changed this and
added a RESULT_FAILED_CONFIGURATION.
api.py:
- 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
These functions are not used to save/restore state. They are solely
used to create and parse p5i files for the "webinstall" functionality
that the GUI (and eventually cli) will have, which is why *not all* of
the information about a publisher or its repositories is written, and
why it is written out in a specific (simplified) way.
The idea is that a user will be able to click on a link to a .p5i file
on a website which will launch a GUI helper that will prompt the user to
add the publisher information contained within to their system and
install any listed packages.
However, the pointer to the memento pattern will be useful when I start
working on updating the cfg_cache file format.
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
set_publisher can be removed; thanks for spotting that.
set_attrs() has to stay for now until the api provides an image-create
interface. I've filed bug 7156 for adding an image management interface
to the client api.
Some of the functions were left in place because I wanted to change the
transport layer as little as possible since you are completely
overhauling it.
publisher.py:
- lines 35-41: What is this for?
I'm assuming you mean they're missing comments. If you're asking what
the actual values are for, they're purely informational at this time.
My guess is that we might be able to somehow leverage them at a later
date; ask Stephen.
For now I've added comments (which were taken directly from the
docstrings for the repository.collection_type property).
- line 60: In what cases don't we want to affix a trailing slash to
the URL?
In the case that a specific page is referenced (legal or informational
URIs):
http://www.company.com/legal.html
^^^^^^^^^^
- 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.
I'd like to leave this as is for now. My assumption was that with your
transport putback, this would change anyway, but at least there would be
a framework in place. At that point we could revisit this and determine
what the best way is to deal with sorting. Is that agreeable?
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 ones that are actually used have been left where necessary and are
intended to be removed in a subsequent putback.
Thanks for the reminder here as there are a significant number of
functions I was able to remove that I had already removed usage of.
The rest looks fine to me. Thanks for doing this.
Thanks for the review,
--
Shawn Walker
_______________________________________________
pkg-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
