Noissue - Append build name to jsshell directory

utils.py

Index: utils.py
diff --git a/utils.py b/utils.py
index c00776e6756469db9a9a49b110cef0e180b8a993..dc41390f7b03fdebf72a013a1d45431ae6efc726 100644
--- a/utils.py
+++ b/utils.py
@@ -27,8 +27,17 @@ JSSHELL_SUPPORTED_PLATFORMS = {
 
 def ensureJSShell():
   baseDir = os.path.dirname(__file__)
-  shell_dir = os.path.join(baseDir, JSSHELL_DIR)
 
+  try:
+    build = JSSHELL_SUPPORTED_PLATFORMS[sys.platform]
+    if isinstance(build, dict):
+      build = build[platform.machine()]
+  except KeyError:
+    raise Exception('Platform %s (%s) not supported by JS shell' % (
+      sys.platform, platform.machine()
+    ))
+
+  shell_dir = os.path.join(baseDir, JSSHELL_DIR + "-" + build)

[Sebastian Noack, 2015/11/24 10:38:43]

Nit: "%s-%s" % (JSSHELL_DIR, build)

[kzar, 2015/11/24 10:43:20]

I think it looks cleaner as is.

os.path.join(baseDir, JSSHELL_DIR + "-" + build)

vs

os.path.join(baseDir, "%s-%s" % (JSSHELL_DIR, build))

[Sebastian Noack, 2015/11/24 10:52:03]

I would generally disagree, if it's only for consistence with other "new" code,
which always uses % when concatenating more than two strings. This is also
faster though that isn't a concern here. But most of the time it's also more
readable. In the case here I see that it's not much a difference in matter of
readability though. And certainly not worth arguing about. LGTM

[Wladimir Palant, 2015/11/24 11:12:40]

I tend to agree with Sebastian here but indeed not important enough.

   if not os.path.exists(shell_dir):
     os.makedirs(shell_dir)
   if sys.platform == 'win32':
@@ -39,15 +48,6 @@ def ensureJSShell():
   if os.path.exists(path):
     return path
 
-  try:
-    build = JSSHELL_SUPPORTED_PLATFORMS[sys.platform]
-    if isinstance(build, dict):
-      build = build[platform.machine()]
-  except KeyError:
-    raise Exception('Platform %s (%s) not supported by JS shell' % (
-      sys.platform, platform.machine()
-    ))
-
   with closing(urllib.urlopen(JSSHELL_URL % build)) as response, \
        zipfile.ZipFile(StringIO(response.read())) as zip:
     zip.extractall(shell_dir)