Issue 6273 - Apply eyeo python code style

third_party/libadblockplus/prepare_dependencies.py

 import sys
 import os
 import shutil
 
+
 def duplicate(src, dst, symlink=False):

[Vasily Kuznetsov, 2018/01/19 13:06:53]

It seems like `symlink` argument is not used in this script. It could be removed
(together with related code) unless some other script imports this function from
here.

[anton, 2018/01/19 13:17:15]

That's true that actually it's not used anywhere.
But i'd leave it as an illustration of that possibility.
What do you think guys?

-  if (os.path.exists(dst)):
-    if (os.path.isdir(dst)):
-      print('Deleting %s' % dst)
-      shutil.rmtree(dst);
+    if os.path.exists(dst):
+        if os.path.isdir(dst):
+            print('Deleting {}'.format(dst))

[Vasily Kuznetsov, 2018/01/19 13:06:53]

There's no `from __future__ import print_function` at the top of the file so I
assume this is Python3-only code. If it's not, that import is needed.

[anton, 2018/01/19 13:17:15]

I'm not Python expert but i'm having python 2.x installed and it works as-is
(without import).

[Vasily Kuznetsov, 2018/01/19 13:35:17]

Yeah, in Python 2 it's parsed as:

    print ("bla bla bla")

The first part is a print statement, the second part is a string in parentheses,
so it works. But it's still cleaner to do the import, because if you have
`print("foo")` and then somebody decides to add something to it: `print("foo",
42)`, this changed line will be parsed as print statement and a tuple, not as a
function call with two arguments. Nobody will die as a result, but it's just
kind of poor hygiene ;)

+            shutil.rmtree(dst)
+        else:
+            # symlink

[Vasily Kuznetsov, 2018/01/19 13:06:53]

Good idea with the comment, but it could also be a regular file, so it will turn
out misleading. Or why can we assume it will be a symlink?

[anton, 2018/01/19 13:17:15]

originally i've tried to symlink the dependencies (they are copied to our
modules from Chromium (another path))
to save the storage. But it turned out that dirs structure have to be different,
so i had to use just copying.
However i still thin that some dependencies could be symlinked
(here is improvement ticket https://issues.adblockplus.org/ticket/6260).

[Vasily Kuznetsov, 2018/01/19 13:35:17]

Acknowledged.

+            os.unlink(dst)
+
+    dst_parent = os.path.abspath(os.path.join(dst, os.pardir))
+    if not os.path.exists(dst_parent):
+        print('Creating parent directory {} for {}'.format(dst_parent, dst))
+        os.makedirs(dst_parent)
+
+    if symlink:
+        print('Symlinking {} to {}'.format(src, dst))
+        os.symlink(src, dst)
     else:
-      # symlink
-      os.unlink(dst)
-      
-  dst_parent = os.path.abspath(os.path.join(dst, os.pardir))
-  if not os.path.exists(dst_parent):
-    print('Creating parent directory %s for %s' % (dst_parent, dst))
-    os.makedirs(dst_parent)
+        print('Copying {} to {}'.format(src, dst))
+        shutil.copytree(src, dst)
 
-  if symlink:
-    print('Symlinking %s to %s' % (src, dst))
-    os.symlink(src, dst)
-  else:
-    print('Copying %s to %s' % (src, dst))
-    shutil.copytree(src, dst)
 
 def prepare_deps():
-  cwd = os.getcwd()
-  libadblockplus_root = os.path.join(cwd, 'src', 'third_party', 'libadblockplus', 'third_party')
-  
-  # googletest
-  # Chromium's googletest has a bit different directories structure ('src' as subdirectory)
-  #googletest_src = os.path.join(cwd, 'src', 'third_party', 'googletest')
-  #googletest_dst = os.path.join(libadblockplus_root, 'googletest')
-  #duplicate(googletest_src, googletest_dst)
+    cwd = os.getcwd()
+    libadblockplus_root = os.path.join(cwd,

[anton, 2018/01/19 07:30:20]

It would be better to rename it to `libadblockplus_third_party` too but it seems
to be unrelated changes.

[Vasily Kuznetsov, 2018/01/19 13:06:53]

Given how much we use `os.path.join` in this function, I wonder if it would make
sense to assign it to a variable, maybe something like `pjoin = os.path.join`.
Then all those lines would need less wrapping and read a bit easier. What do you
think?

[anton, 2018/01/19 13:17:14]

Great ideas. However we will save just about 7 characters and it seems to be not
very helpful.

[Vasily Kuznetsov, 2018/01/19 13:35:17]

Acknowledged.

[sergei, 2018/01/19 14:19:43]

What about calling os.path.join only when it's really needed and meanwhile use a
tuple/list?

+                                       'src', 'third_party',
+                                       'libadblockplus', 'third_party')
 
-  # gyp
-  # Chromium's gyp can't be used because of compile error:
-  # *** No rule to make target `shell/src/Main.cpp', needed by `local/armeabi-v7a/objs/abpshell/shell/src/Main.o'.  Stop.
-  # See. https://hg.adblockplus.org/gyp/rev/fix-issue-339
-  #gyp_src = os.path.join(cwd, 'src', 'tools', 'gyp')
-  #gyp_dst = os.path.join(libadblockplus_root, 'gyp')
-  #duplicate(gyp_src, gyp_dst)
+    # googletest
+    #
+    # Chromium's googletest has a bit different directories structure
+    # ('src' as subdirectory)
+    #
+    # googletest_src = os.path.join(cwd, 'src', 'third_party', 'googletest')

[Vasily Kuznetsov, 2018/01/19 13:06:52]

This commented out code (as well as other below) with some info about why we
can't do that is confusing. Are these really helpful comments? Maybe we can just
delete them or move them to some kind of documentation file?

[anton, 2018/01/19 13:17:15]

Agree, i tend to think we should remove the commented out lines and just leave
comments.
What do you think guys?

[sergei, 2018/01/19 14:19:43]

Just remove everything and leave a comment if it's required explaining the
reason it's done for what it's done. Basically comment the existing code, not a
potential code.

+    # googletest_dst = os.path.join(libadblockplus_root, 'googletest')
+    # duplicate(googletest_src, googletest_dst)
 
-  # back-up /v8/testing/gtest as it will be deleted while preparing v8
-  gtest_bak_src = os.path.join(libadblockplus_root, 'v8', 'testing', 'gtest')
-  gtest_bak_dst = os.path.join(libadblockplus_root, 'gtest.bak')
-  shutil.move(gtest_bak_src, gtest_bak_dst)
+    # gyp
+    #
+    # Chromium's gyp can't be used because of compile error:
+    # *** No rule to make target `shell/src/Main.cpp',
+    # needed by `local/armeabi-v7a/objs/abpshell/shell/src/Main.o'.  Stop.
+    # See. https://hg.adblockplus.org/gyp/rev/fix-issue-339
+    #
+    # gyp_src = os.path.join(cwd, 'src', 'tools', 'gyp')
+    # gyp_dst = os.path.join(libadblockplus_root, 'gyp')
+    # duplicate(gyp_src, gyp_dst)
 
-  # v8
-  v8_src = os.path.join(cwd, 'src', 'v8')
-  v8_dst = os.path.join(libadblockplus_root, 'v8')
-  duplicate(v8_src, v8_dst)
+    # back-up /v8/testing/gtest as it will be deleted while preparing v8
+    gtest_bak_src = os.path.join(libadblockplus_root, 'v8', 'testing', 'gtest')
+    gtest_bak_dst = os.path.join(libadblockplus_root, 'gtest.bak')
+    shutil.move(gtest_bak_src, gtest_bak_dst)
 
-  # restore /v8/testing/gtest from backup
-  shutil.move(gtest_bak_dst, gtest_bak_src)
+    # v8
+    v8_src = os.path.join(cwd, 'src', 'v8')
+    v8_dst = os.path.join(libadblockplus_root, 'v8')
+    duplicate(v8_src, v8_dst)
 
-  # v8/base/trace_event/common
-  trace_common_src = os.path.join(cwd, 'src', 'base', 'trace_event', 'common')
-  trace_common_dst = os.path.join(libadblockplus_root, 'v8', 'base', 'trace_event', 'common')
-  duplicate(trace_common_src, trace_common_dst)
+    # restore /v8/testing/gtest from backup
+    shutil.move(gtest_bak_dst, gtest_bak_src)
 
-  # v8/build
-  build_src = os.path.join(cwd, 'src', 'build')
-  build_dst = os.path.join(libadblockplus_root, 'v8', 'build')
-  duplicate(build_src, build_dst)
+    # v8/base/trace_event/common
+    trace_common_src = os.path.join(cwd,
+                                    'src', 'base', 'trace_event', 'common')
+    trace_common_dst = os.path.join(libadblockplus_root,
+                                    'v8', 'base', 'trace_event', 'common')
+    duplicate(trace_common_src, trace_common_dst)
 
-  # v8/testing/gtest
-  # Chromium's gtest can't be used becuase of compile error:
-  # fatal error: third_party/googletest/src/googletest/include/gtest/gtest_prod.h: No such file or directory
-  # #include "third_party/googletest/src/googletest/include/gtest/gtest_prod.h"
-  # so we have to back-up gtest above and restore it after prepring of 'v8' directory
-  #gtest_src = os.path.join(cwd, 'src', 'testing', 'gtest')
-  #gtest_dst = os.path.join(libadblockplus_root, 'v8', 'testing', 'gtest')
-  #duplicate(gtest_src, gtest_dst)
+    # v8/build
+    build_src = os.path.join(cwd, 'src', 'build')
+    build_dst = os.path.join(libadblockplus_root, 'v8', 'build')
+    duplicate(build_src, build_dst)
 
-  # v8/tools/gyp
-  tools_gyp_src = os.path.join(cwd, 'src', 'tools', 'gyp')
-  tools_gyp_dst = os.path.join(libadblockplus_root, 'v8', 'tools', 'gyp')
-  duplicate(tools_gyp_src, tools_gyp_dst)
+    # v8/testing/gtest
+    #
+    # Chromium's gtest can't be used becuase of compile error:
+    # fatal error: third_party/googletest/src/googletest/include/gtest/gtest_prod.h: No such file or directory
+    # #include "third_party/googletest/src/googletest/include/gtest/gtest_prod.h"
+    # so we have to back-up gtest above and restore it
+    # after prepring of 'v8' directory
+    #
+    # gtest_src = os.path.join(cwd, 'src', 'testing', 'gtest')
+    # gtest_dst = os.path.join(libadblockplus_root, 'v8', 'testing', 'gtest')
+    # duplicate(gtest_src, gtest_dst)
 
-  # v8/tools/clang
-  clang_src = os.path.join(cwd, 'src', 'tools', 'clang')
-  clang_dst = os.path.join(libadblockplus_root, 'v8', 'tools', 'clang')
-  duplicate(clang_src, clang_dst)
+    # v8/tools/gyp

[Vasily Kuznetsov, 2018/01/19 13:06:52]

Lines 91-112 could be rolled into a loop -- all five triples of lines basically
do the same thing. This would make this part shorter and a bit easier to read at
a glance.

[anton, 2018/01/19 13:17:14]

did you mean iterating over array of 'gyp','clang','icu','jinja2' ?

[Vasily Kuznetsov, 2018/01/19 13:35:17]

Yeah, something like this:

    for path in [
        ('tools', 'gyp'),
        ('tools', 'clang),
        ('third_party', 'icu'),
        ...
    ]:
        src = os.path.join(cwd, 'src', *path)
        dst = os.path.join(libadblockplus_root, 'v8', *path)
        duplicate(src, dst)

+    tools_gyp_src = os.path.join(cwd, 'src', 'tools', 'gyp')
+    tools_gyp_dst = os.path.join(libadblockplus_root, 'v8', 'tools', 'gyp')
+    duplicate(tools_gyp_src, tools_gyp_dst)
 
-  # v8/third_party/icu
-  icu_src = os.path.join(cwd, 'src', 'third_party', 'icu')
-  icu_dst = os.path.join(libadblockplus_root, 'v8', 'third_party', 'icu')
-  duplicate(icu_src, icu_dst)
+    # v8/tools/clang
+    clang_src = os.path.join(cwd, 'src', 'tools', 'clang')
+    clang_dst = os.path.join(libadblockplus_root, 'v8', 'tools', 'clang')
+    duplicate(clang_src, clang_dst)
 
-  # v8/third_party/jinja2
-  jinja2_src = os.path.join(cwd, 'src', 'third_party', 'jinja2')
-  jinja2_dst = os.path.join(libadblockplus_root, 'v8', 'third_party', 'jinja2')
-  duplicate(jinja2_src, jinja2_dst)
+    # v8/third_party/icu
+    icu_src = os.path.join(cwd, 'src', 'third_party', 'icu')
+    icu_dst = os.path.join(libadblockplus_root, 'v8', 'third_party', 'icu')
+    duplicate(icu_src, icu_dst)
 
-  # v8/third_party/markupsafe
-  markupsafe_src = os.path.join(cwd, 'src', 'third_party', 'markupsafe')
-  markupsafe_dst = os.path.join(libadblockplus_root, 'v8', 'third_party', 'markupsafe')
-  duplicate(markupsafe_src, markupsafe_dst)
+    # v8/third_party/jinja2
+    jinja2_src = os.path.join(cwd, 'src', 'third_party', 'jinja2')
+    jinja2_dst = os.path.join(libadblockplus_root,
+                              'v8', 'third_party', 'jinja2')
+    duplicate(jinja2_src, jinja2_dst)
 
-  return 0  
+    # v8/third_party/markupsafe
+    markupsafe_src = os.path.join(cwd, 'src', 'third_party', 'markupsafe')
+    markupsafe_dst = os.path.join(libadblockplus_root,
+                                  'v8', 'third_party', 'markupsafe')
+    duplicate(markupsafe_src, markupsafe_dst)
+
+    return 0
+
 
 def main(argv):
-  return prepare_deps();
+    return prepare_deps()
+
 
 if '__main__' == __name__:

[Vasily Kuznetsov, 2018/01/19 13:06:52]

Yoda-conditions are generally frowned upon in Python world. Since `if foo =
'bar':` is not valid anyway, we don't gain additional error protection this way
and only make readability somewhat worse. I would recommend rewriting this as
`if __name__ == '__main__':`.

[anton, 2018/01/19 13:17:15]

Acknowledged.

-  try:
-    sys.exit(main(sys.argv[1:]))
-  except KeyboardInterrupt:
-    sys.stderr.write('interrupted\n')
-    sys.exit(1)
+    try:
+        sys.exit(main(sys.argv[1:]))

[Vasily Kuznetsov, 2018/01/19 13:06:53]

Since `main()` always returns the result of `prepare_deps()` and
`prepare_deps()` seems to always return 0 (which is the default return code for
Python scripts), we don't need to call `sys.exit()` and pass the return value of
`main()` to it. Also that `return` in line 118 would not be needed.

[Vasily Kuznetsov, 2018/01/19 13:06:53]

It seems like we could just call `prepare_deps()` directly here -- `main()` only
delegates to it and ignores the argument.

[anton, 2018/01/19 13:17:14]

Acknowledged.

+    except KeyboardInterrupt:
+        sys.stderr.write('interrupted\n')

[Vasily Kuznetsov, 2018/01/19 13:06:52]

You can just use `sys.exit('interrupted')` to achieve the same effect as these
two lines.

[anton, 2018/01/19 13:17:15]

Acknowledged.

+        sys.exit(1)