[webkit-reviews] review denied: [Bug 124848] Add support for multiple sources for AutoInstaller : [Attachment 217818] Support for multiple sources in the autoinstaller of webkitpy.

[bugzilla-daemon at webkit.org]

Ryosuke Niwa <rniwa at webkit.org> has denied Berta József
<jberta at inf.u-szeged.hu>'s request for review:
Bug 124848: Add support for multiple sources for AutoInstaller
https://bugs.webkit.org/show_bug.cgi?id=124848

Attachment 217818: Support for multiple sources in the autoinstaller of
webkitpy.
https://bugs.webkit.org/attachment.cgi?id=217818&action=review

------- Additional Comments from Ryosuke Niwa <rniwa at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=217818&action=review


> Tools/Scripts/webkitpy/common/system/autoinstall.py:51
> +_mirror_regexs = re.compile('.*sourceforge.*'), re.compile('.*pypi.*')

Why don't we have a single array that contains both the regular expression and
the env. variable name instead of zip'ing in _prepare_mirrors?

> Tools/Scripts/webkitpy/common/system/autoinstall.py:53
> +_sf_env_var = 'SOURCEFORGE_MIRRORS'

Please don't use abbreviations such as sf. Spell out source forge.

> Tools/Scripts/webkitpy/common/system/autoinstall.py:269
> +    def _copy_not_packaged(self, path, scratch_dir):

s/not_packaged/unpackaged_files/

> Tools/Scripts/webkitpy/common/system/autoinstall.py:270
> +	   """Copy the not packaged file to the scratch_dir in order to leave
the local cache intact. """

We no longer add these Python-style one-line comments. That's reminiscent of
Google-owned code.
Please give the method descriptive name such as
copy_unpackaged_file_from_local_cache to avoid having to add these comments.

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

What does mean to say "prepare" mirrors?
It's better to give a more descriptive method name such as mirrors_from_env.

> Tools/Scripts/webkitpy/common/system/autoinstall.py:320
> +    def _switch_to_mirror(self, url, mirrors, mirror_index):

Again, _switch_to_mirror is not the most descriptive method name here.

> Tools/Scripts/webkitpy/common/system/autoinstall.py:325
> +	   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 description belongs in _prepare_mirrors.

> Tools/Scripts/webkitpy/common/system/autoinstall.py:359
> +		       url = self._switch_to_mirror(url, mirrors, mirror_index)

> +		       if url:
> +			   failures = 0
> +			   mirror_index += 1
> +			   continue

It's strange that we'll select the next mirror only if the current mirror
matched.
How does that work?

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

It's better to call this _find_in_local_autoinstall_cache to be more explicit.

> Tools/Scripts/webkitpy/common/system/autoinstall.py:387
> +	   """The installer first looks at the local cache, and if the package
is found there, it will be installed from there.
> +	   The path of the cache is stored in the LOCAL_AUTOINSTALL_CACHE
environment variable.
> +	   The cache must contain the packages in their packed forms(As if they
were just downloaded from the web)
> +	   """

Doesn't seem like this is the description of this method.  It's better to add a
comment in the relevant parts of _download as needed.