Merge lp://staging/~netmask/smart/mirrorlist into lp://staging/smart

This is looking interesting. Here are a few comments on the
implementation:

[1]

+ def __init__(self, baseurl, mirrorlist = None, *args):

Default parameters shouldn't have spaces around the equals sign.

[2]

+ sysconf.add(("mirrors", self._baseurl), mirror, unique=True)

This is polluting the custom user-provided set of mirrors. This means
that once the mirror list from this channel is loaded, it will not go
away anymore. Instead of doing this, check out how the MirrorChannel
does it.

You may have to do a few minor tweaks to enable this channel to use the
same "getMirrors" logic. Specifically:

1. Implement a default getMirrors() method in the Channel base class,
   which returns an empty dictionary.

2. Change the use of getMirrors() in control.py so that it doesn't check
   to see if the channel is a MirrorsChannel instance or not. This way
   your channel will also be considered.

[3]

+ except:
+ iface.warning(_("Could not load mirror list. Continuing with base URL only."))
+ pass

"Bare excepts" (excepts without an explicit exception type) are not
a good idea most of the times. This will catch even syntax error,
keyboard interrupts, and so on. Instead, wrap only the lines you
really expect an error on, and say what exception you're expecting
there explicitly.

Also, the "pass" isn't necessary.

[4]

+ if self._mirrorlist:
+ mirrors = self._mirrorlist
+ item = fetcher.enqueue(mirrors)
+ fetcher.run(progress=progress)

Why is the fetcher being run explicitly here? It looks like the item
could be enqueued normally, and fetched together with all the other
files.

The only reason to run the fetcher beforehand is when the information
you'll fetch again later depends on the information being fetched now
(e.g. you don't have to download all the metadata files if the digest
file matches the previously downloaded one).

[5]

+ >>> sysconf.get("mirrors", channel._baseurl)

When a private attribute is accessed in a test case, it becomes a
"whitebox test", meaning that the test has access to the internal
implementation. This is generally a bad idea, since it ties the
internal implementation to the test case.

In the case above, we know what the base url must be, so we should
use it explicitly instead of turning the test into a whitebox one.

Em Ter, 2008-07-22 às 14:03 +0000, Gustavo Niemeyer escreveu:

> Why is the fetcher being run explicitly here? It looks like the item
> could be enqueued normally, and fetched together with all the other
> files.

This was an implementation choice. That way the next files (including
the repodata.xml) can be loaded using mirrors. This is a common
implementation in Yum, by which you can simply have mirrorlist URL but
no baseurl in the repository information. I think they implemented that
way to they reduce the load in the main server.

> + >>> sysconf.get("mirrors", channel._baseurl)

The test case has been completelly rewritten based on the proposal of
changing the load format.

All the changes have been commited. Please review.

Thanks for the comments.

Any new comments?