[Webkit-unassigned] [Bug 124848] Add support for multiple sources for AutoInstaller

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=124848





--- Comment #8 from Ryosuke Niwa <rniwa at webkit.org>  2014-01-07 01:29:24 PST ---
(From update of attachment 218945)
View in context: https://bugs.webkit.org/attachment.cgi?id=218945&action=review

> Tools/ChangeLog:1
> +

Nit: remove this empty line.

> Tools/Scripts/webkitpy/common/system/autoinstall.py:305
> +    def _get_mirrors_from_env(self):

get prefix is used for functions with an out arguments. See our style guideline.

> Tools/Scripts/webkitpy/common/system/autoinstall.py:306
> +        """If the original download url fails, it is replaced by a mirror provided in the environment variables

This is what the caller of this function does.  I don't think we need this comment here.  Please remove it.

> Tools/Scripts/webkitpy/common/system/autoinstall.py:310
> +        The environment variables must contain the values separated by colons.
> +        Pypi mirror examle: PYPI_MIRRORS=pypi.hustunique.com:e.pypi.python.org...
> +        Sourceforge mirror example: SOURCEFORGE_MIRRORS=aarnet.dl.sourceforge.net:citylan.dl.sourceforge.net:dfn.dl.sourceforge.net:freefr.dl.sourceforge.net...
> +        Mirror sources: http://www.pypi-mirrors.org/, http://sourceforge.net/apps/trac/sourceforge/wiki/Mirrors

This is an excessive amount of comments/examples for a very simple example.

It's much better to convey this information via a more descriptive function name such as
parse_colon_separated_mirrors_from_env

> Tools/Scripts/webkitpy/common/system/autoinstall.py:327
> +        parsed_url = list(urlparse.urlparse(url))

Why are we converting the parsed result into a list?  That's reducing the readability of the code below.

> Tools/Scripts/webkitpy/common/system/autoinstall.py:353
> +                else:

Since the previous if statement ends with continue, we shouldn't have else clause. See our style guideline.

> Tools/Scripts/webkitpy/common/system/autoinstall.py:381
> +    def _find_in_local_autoinstall_cache(self, filename):

I don't think we if should use the prefix "find" given that this function doesn't search through the file system.

> Tools/Scripts/webkitpy/common/system/autoinstall.py:384
> +        # The cache must contain the packages in their packed forms, as if they were just downloaded from the web.
> +
> +        # The path of the cache is stored in the environment variable named in the _CACHE_ENV_VAR variable.

These two comments are more confusing than helpful as far as I read the code. Please remove them.

> Tools/Scripts/webkitpy/common/system/autoinstall.py:385
> +        if _CACHE_ENV_VAR in os.environ:

We prefer an early exit. e.g.
if _CACHE_ENV_VAR not in os.environ:
    return False
...
if not path:
    return False
return ...

> Tools/Scripts/webkitpy/common/system/autoinstall.py:388
> +            path = glob(os.path.join(os.environ[_CACHE_ENV_VAR], filename) + '*')
> +            if path:
> +                return path[0]

Why are we returning the first path?

> Tools/Scripts/webkitpy/common/system/autoinstall.py:390
> +        else:
> +            return False

These two lines are complete no-op as is.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.