On Sun, Jan 19, 2014 at 02:36:43PM -0800, Alec Warner wrote: > On Sat, Jan 18, 2014 at 7:07 PM, W. Trevor King <wk...@tremily.us> wrote: > > +def _get_file_uri_tuples(uris): > > + """Return a list of (filename, uri) tuples > > + """ > > > > As mike noted on another thread: > > ""Return a list of (filename, uri) tuples."""
I'd replaced this with:
"""
Return a list of (filename, uri) tuples
"""
in v2, but I'll queue the one-line form in v3.
> > +def _expand_mirror(uri, custom_mirrors=(), third_party_mirrors=()):
> > + """Replace the 'mirror://' scheme in the uri
> >
>
> Sentence should end in a period.
Ok. I'll do that for the other summaries as well, and uppercase URI,
in v3.
> > + writemsg(_("Invalid mirror definition in SRC_URI:\n"),
> > + noiselevel=-1)
> > + writemsg(" %s\n" % (uri), noiselevel=-1)
>
>
> Is this a py3k thing? You should be able to write:
>
> writemsg(" %s\n" % uri, noiselevel=-1)
>
> The tuple is only needed for multiple arguments in py2k.
1. I think I just copied and pasted it ;).
2. (uri) is not actually a tuple:
>>> type(('hello'))
<type 'str'>
To get a tuple, you need (uri,).
I'm fine removing the parens in v3.
> > +def _get_uris(uris, settings, custom_mirrors=(), locations=()):
> >
>
> I want you to be careful here. This is bad style when you are
> constructing mutable objects in parameter defaults:
I used tuples instead of lists because they are not mutable (as you
point out later on).
> I'm not sure if I appreciate that more or less than the other form....
>
> def _get_uris(uris, settings, custom_mirrors=None, locations=None):
> if not custom_mirrors:
> custom_mirrors = ()
> if not locations:
> locations = ()
If you want me to do it that way, I'll queue it for v3. I think the
default tuples are more readable. They are certainly shorter.
> Another advisable way to write it is to simply not have default
> arguments, and to force the caller to sync in an empty iterable:
Meh. I don't have strong feelings about this, but custom_mirrors
struck me as something that would not always be required ;).
> > + third_party_mirrors = settings.thirdpartymirrors()
> >
>
> Why pass in all of settings?
We need other stuff too, see v2. The less settings parsing I need to
do in fetch() itself, the happier I am. If that makes the helper
functions a bit uglier, that's fine. Fewer people will have to read
those.
> Think about testing this function.
Working on it for v3.
> Do you really want to try to construct some sort of 'testing'
> settings object, or simply construct a list?
I was going to use a dict for testing, and stub out a
thirdpartymirrors mock for _get_uris.
> > + third_party_mirror_uris = {}
> > + filedict = OrderedDict()
> > + primaryuri_dict = {}
> > + for filename, uri in _get_file_uri_tuples(uris=uris):
> > + if filename not in filedict:
> > + filedict[filename] = [
> > + os.path.join(location, 'distfiles',
> > filename)
> > + for location in locations]
> > + if uri is None:
> > + continue
> > + if uri.startswith('mirror://'):
> > + uris = _expand_mirror(
> >
>
> too many uri / uris variables...
>
> can we called this 'expanded_uris'?
Queued for v3.
> I'm really having trouble tracking what is what. Remember that
> 'uris' is already a parameter to this function. Are you shadowing it
> here, or trying to overwrite it?
Clobbering it. The original value is only used for the
_get_file_uri_tuples call.
> > + uri=uri, custom_mirrors=custom_mirrors,
> > + third_party_mirrors=third_party_mirrors)
> > + filedict[filename].extend(uri for group, uri in
> > uris)
> >
>
> group appears unused in your implicit iterable here.
>
> perhaps:
>
> filedict[filename].extend(uri for _, uri in uris)
Queued for v3.
> > + third_party_mirror_uris.setdefault(filename,
> > []).extend(
> > + uri for group, uri in uris
> > + if group == 'third-party')
> >
>
> I'm curious if this matters. We are iterator over 'uris' twice. Is it
> cheaper to do it once and build the iterables once?
>
> third_party_uris = []
> for group, uri in uris:
> if group == 'third_party':
> third_party_uris.append(uri)
> filedict[filename].append(uri)
>
> I'm guessing the iterable is so small it doesn't matter.
My guess is that the speed gain from list comprehension outweighs the
loss of double iterations, but I agree that it is probably doesn't
matter ;).
> > + if not filedict[filename]:
> > + writemsg(
> > + _("No known mirror by the name:
> > %s\n")
> > + % (mirror,))
> > + else:
> > + if restrict_fetch or force_mirror:
> >
>
> Are these globals or am I missing somethng?
Fixed in v2.
> > + # Only fetch from specific mirrors is
> > allowed.
> > + continue
> > + primaryuris = primaryuri_dict.get(filename)
> > + if primaryuris is None:
> > + primaryuris = []
> > + primaryuri_dict[filename] = primaryuris
> > + primaryuris.append(uri)
> > +
> > + # Order primaryuri_dict values to match that in SRC_URI.
> > + for uris in primaryuri_dict.values():
> > + uris.reverse()
>
> Is there any guaranteed ordering for dict.values()?
No.
> I thought dict order was random (and they seriously make it random
> in modern python versions, to detect bugs.) How does this
> uris.reverse() match the SRC_URI ordering?
No idea (copy-pasted code). I'd be happy to cut this out in v3.
> > +
> > + # Prefer third_party_mirrors over normal mirrors in cases when
> > + # the file does not yet exist on the normal mirrors.
> > + for filename, uris in third_party_mirror_uris.items():
> > + primaryuri_dict.setdefault(filename, []).extend(uris)
> > +
> > + # Now merge primaryuri values into filedict (includes mirrors
> > + # explicitly referenced in SRC_URI).
> > + if "primaryuri" in restrict:
> >
>
> same questoin here about where 'restrict' comes from.
Fixed in v2.
Thanks,
Trevor
--
This email may be signed or encrypted with GnuPG (http://www.gnupg.org).
For more information, see http://en.wikipedia.org/wiki/Pretty_Good_Privacy
signature.asc
Description: OpenPGP digital signature
