Issue 29390650: Issue 5020 - Part 1: Make compile script PEP8-compliant (Closed)
|
Created:
March 21, 2017, 11:22 a.m. by Wladimir Palant Modified:
March 27, 2017, 7:59 a.m. CC:
hub Base URL:
https://hg.adblockplus.org/adblockpluscore Visibility:
Public. |
DescriptionIssue 5020 - Part 1: Make compile script PEP8-compliant
Patch Set 1 #
Total comments: 6
Patch Set 2 : Fixed string formatting #MessagesTotal messages: 8
Hi Wladimir, The changes look good but I also wanted to run flake8 over them just in case I missed something. Then I ran into a problem: this patch doesn't apply to either the master bookmark or the tip of the default branch. What is the base revision for this change? Cheers, Vasily
https://codereview.adblockplus.org/29390650/diff/29390651/compile File compile (right): https://codereview.adblockplus.org/29390650/diff/29390651/compile#newcode42 compile:42: '/bin/bash', '-c', os.path.join(EMSCRIPTEN_PATH, 'emsdk_env.sh') Unrelated of coding style changes but note that subprocess searches PATH so the absolute path is redundant (and less portable), than: output = subprocess.check_output(['bash', ...]) If the script in emsdk_env.sh is executable, following would work as well: output = subprocess.check_output(['os.path.join(EMSCRIPTEN_PATH, 'emsdk_env.sh'), ...]) https://codereview.adblockplus.org/29390650/diff/29390651/compile#newcode77 compile:77: params.extend(['-s', '%s=%s' % (key, str(value))]) Please use format instead of % when for string formatting. flake8-eyeo would have complained about that, and might potentially find more issues that I'm overlooking. Mind running flake with the falke8-eyeo extension on this script?
On 2017/03/21 12:33:57, Vasily Kuznetsov wrote: > The changes look good but I also wanted to run flake8 over them just in case I > missed something. Then I ran into a problem: this patch doesn't apply to either > the master bookmark or the tip of the default branch. What is the base revision > for this change? The file currently only exists on the emscripten branch, and there is https://codereview.adblockplus.org/29383816/ between the current repository state and this change. I did run flake8 before uploading the review however, so all issues have been addressed.
https://codereview.adblockplus.org/29390650/diff/29390651/compile File compile (right): https://codereview.adblockplus.org/29390650/diff/29390651/compile#newcode42 compile:42: '/bin/bash', '-c', os.path.join(EMSCRIPTEN_PATH, 'emsdk_env.sh') On 2017/03/21 12:41:01, Sebastian Noack wrote: > Unrelated of coding style changes but note that subprocess searches PATH so the > absolute path is redundant (and less portable), than: While I generally dislike relying on PATH, this code is going to be replaced in Part 4 anyway - we will call emsdk instead, that one is executable directly. https://codereview.adblockplus.org/29390650/diff/29390651/compile#newcode77 compile:77: params.extend(['-s', '%s=%s' % (key, str(value))]) On 2017/03/21 12:41:01, Sebastian Noack wrote: > Please use format instead of % when for string formatting. flake8-eyeo would > have complained about that, and might potentially find more issues that I'm > overlooking. Mind running flake with the falke8-eyeo extension on this script? Done.
https://codereview.adblockplus.org/29390650/diff/29390651/compile File compile (right): https://codereview.adblockplus.org/29390650/diff/29390651/compile#newcode20 compile:20: 'SHELL_FILE': "'%s'" % os.path.abspath(os.path.join(SOURCE_DIR, Funnily enough here flake8-abp doesn't complain about % vs .format() but the same reasoning as for line 77 would apply. https://codereview.adblockplus.org/29390650/diff/29390651/compile#newcode77 compile:77: params.extend(['-s', '%s=%s' % (key, str(value))]) On 2017/03/21 12:41:01, Sebastian Noack wrote: > Please use format instead of % when for string formatting. flake8-eyeo would > have complained about that, and might potentially find more issues that I'm > overlooking. Mind running flake with the falke8-eyeo extension on this script? I have done it and it only gives A107 on this line.
LGTM for patch set 2
LGTM 
|
