Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code

Issue 29330730: Noissue - Append build name to jsshell directory (Closed)

Created:
Nov. 24, 2015, 10:31 a.m. by kzar
Modified:
Nov. 24, 2015, 2:31 p.m.
Visibility:
Public.

Description

Noissue - Append build name to jsshell directory

Patch Set 1 #

Total comments: 4
Unified diffs Side-by-side diffs Delta from patch set Stats (+10 lines, -10 lines) Patch
M utils.py View 2 chunks +10 lines, -10 lines 4 comments Download

Messages

Total messages: 5
kzar
Patch Set 1
Nov. 24, 2015, 10:32 a.m. (2015-11-24 10:32:56 UTC) #1
Sebastian Noack
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: ...
Nov. 24, 2015, 10:38 a.m. (2015-11-24 10:38:44 UTC) #2
kzar
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 ...
Nov. 24, 2015, 10:43 a.m. (2015-11-24 10:43:21 UTC) #3
Sebastian Noack
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 ...
Nov. 24, 2015, 10:52 a.m. (2015-11-24 10:52:04 UTC) #4
Wladimir Palant
Nov. 24, 2015, 11:12 a.m. (2015-11-24 11:12:40 UTC) #5
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.

Powered by Google App Engine
This is Rietveld