Noissue - Append build name to jsshell directory

Noissue - Append build name to jsshell directory

[kzar, 2015-11-24 10:32:56]

Patch Set 1

[Sebastian Noack, 2015-11-24 10:38:44]

https://codereview.adblockplus.org/29330730/diff/29330731/utils.py
File utils.py (right):

https://codereview.adblockplus.org/29330730/diff/29330731/utils.py#newcode40
utils.py:40: shell_dir = os.path.join(baseDir, JSSHELL_DIR + "-" + build)
Nit: "%s-%s" % (JSSHELL_DIR, build)

[kzar, 2015-11-24 10:43:21]

https://codereview.adblockplus.org/29330730/diff/29330731/utils.py
File utils.py (right):

https://codereview.adblockplus.org/29330730/diff/29330731/utils.py#newcode40
utils.py:40: shell_dir = os.path.join(baseDir, JSSHELL_DIR + "-" + build)
On 2015/11/24 10:38:43, Sebastian Noack wrote:
> Nit: "%s-%s" % (JSSHELL_DIR, build)

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:04]

https://codereview.adblockplus.org/29330730/diff/29330731/utils.py
File utils.py (right):

https://codereview.adblockplus.org/29330730/diff/29330731/utils.py#newcode40
utils.py:40: shell_dir = os.path.join(baseDir, JSSHELL_DIR + "-" + build)
On 2015/11/24 10:43:20, kzar wrote:
> On 2015/11/24 10:38:43, Sebastian Noack wrote:
> > Nit: "%s-%s" % (JSSHELL_DIR, build)
> 
> I think it looks cleaner as is.
> 
> os.path.join(baseDir, JSSHELL_DIR + "-" + build)
> 
> vs
> 
> os.path.join(baseDir, "%s-%s" % (JSSHELL_DIR, build))

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]

LGTM

https://codereview.adblockplus.org/29330730/diff/29330731/utils.py
File utils.py (right):

https://codereview.adblockplus.org/29330730/diff/29330731/utils.py#newcode40
utils.py:40: shell_dir = os.path.join(baseDir, JSSHELL_DIR + "-" + build)
On 2015/11/24 10:52:03, Sebastian Noack wrote:
> I would generally disagree, if it's only for consistence with other "new"
code,
> which always uses % when concatenating more than two strings.

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