Add a run_eslint wrapper script

Add a run_eslint wrapper script

In doing so, I also moved run_eslint to common/eslint.

With no arguments, this script lints all dashboard/ and tracing/ HTML
files. When passed arguments, it assumes those arguments are the files
to be linted.

BUG=catapult:#2810

Committed: https://chromium.googlesource.com/external/github.com/catapult-project/catapult/+/e1b701cbabeeac404049a83ffccd56f667c3ae12

[charliea (OOO until 10-5), 2016-09-23 03:16:18]

Patchset #1 (id:1) has been deleted

[charliea (OOO until 10-5), 2016-09-23 03:18:23]

charliea@chromium.org changed reviewers:
+ nednguyen@google.com

[charliea (OOO until 10-5), 2016-09-23 03:18:23]

Ned, would you mind reviewing this? (Ethan's OOO until 9/27, and I'm OOO all of
next week)

https://codereview.chromium.org/2361623007/diff/20001/common/node_runner/bin/...
File common/node_runner/bin/test_node_for_smoke (left):

https://codereview.chromium.org/2361623007/diff/20001/common/node_runner/bin/...
common/node_runner/bin/test_node_for_smoke:13: _PY_UTILS_PATH = os.path.abspath(
(I think that this was unused)

[nednguyen, 2016-09-23 10:17:49]

https://codereview.chromium.org/2361623007/diff/20001/catapult_build/js_check...
File catapult_build/js_checks.py (right):

https://codereview.chromium.org/2361623007/diff/20001/catapult_build/js_check...
catapult_build/js_checks.py:4: 
Hmhh, shouldn't file be moved to eslint module?

https://codereview.chromium.org/2361623007/diff/20001/common/eslint/eslint/es...
File common/eslint/eslint/eslint.py (right):

https://codereview.chromium.org/2361623007/diff/20001/common/eslint/eslint/es...
common/eslint/eslint/eslint.py:4: 
Move the content of this file to __init__.py instead of having
"eslint/eslint/eslint.py"

https://codereview.chromium.org/2361623007/diff/20001/common/eslint/eslint/es...
common/eslint/eslint/eslint.py:30: def RunEslintGlobal():
Can you merge this to RunEslintOnDir(dir_path), the DIRECTORIES_TO_LINT should
be moved to the script size & not be hardcoded in the module

https://codereview.chromium.org/2361623007/diff/20001/common/node_runner/bin/...
File common/node_runner/bin/test_node_for_smoke (right):

https://codereview.chromium.org/2361623007/diff/20001/common/node_runner/bin/...
common/node_runner/bin/test_node_for_smoke:1: #!/usr/bin/env python
We can now remove this & change the catapult_build/build_steps.py to run the	
common/eslint/bin/run_eslint instead. 

Having a circular dependency between node_runner & eslint is bad

[charliea (OOO until 10-5), 2016-09-23 13:02:32]

Patchset #2 (id:40001) has been deleted

[charliea (OOO until 10-5), 2016-09-23 13:03:51]

Patchset #2 (id:60001) has been deleted

[charliea (OOO until 10-5), 2016-09-23 13:03:51]

Patchset #2 (id:60001) has been deleted

[charliea (OOO until 10-5), 2016-09-23 13:06:35]

PTAL

https://codereview.chromium.org/2361623007/diff/20001/catapult_build/js_check...
File catapult_build/js_checks.py (right):

https://codereview.chromium.org/2361623007/diff/20001/catapult_build/js_check...
catapult_build/js_checks.py:4: 
On 2016/09/23 at 10:17:49, nednguyen wrote:
> Hmhh, shouldn't file be moved to eslint module?

IDTS: there are still non-eslint-based javascript checks that we do, like making
sure that "const" is never used. Eventually, we should kill all of these
non-eslint-based checks, but for now, I think it makes sense to have them done
outside of the eslint/ directory.

https://codereview.chromium.org/2361623007/diff/20001/common/eslint/eslint/es...
File common/eslint/eslint/eslint.py (right):

https://codereview.chromium.org/2361623007/diff/20001/common/eslint/eslint/es...
common/eslint/eslint/eslint.py:4: 
On 2016/09/23 at 10:17:49, nednguyen wrote:
> Move the content of this file to __init__.py instead of having
"eslint/eslint/eslint.py"

Done.

https://codereview.chromium.org/2361623007/diff/20001/common/eslint/eslint/es...
common/eslint/eslint/eslint.py:4: 
On 2016/09/23 at 10:17:49, nednguyen wrote:
> Move the content of this file to __init__.py instead of having
"eslint/eslint/eslint.py"

Done.

https://codereview.chromium.org/2361623007/diff/20001/common/eslint/eslint/es...
common/eslint/eslint/eslint.py:30: def RunEslintGlobal():
On 2016/09/23 at 10:17:49, nednguyen wrote:
> Can you merge this to RunEslintOnDir(dir_path), the DIRECTORIES_TO_LINT should
be moved to the script size & not be hardcoded in the module

Done.

https://codereview.chromium.org/2361623007/diff/20001/common/eslint/eslint/es...
common/eslint/eslint/eslint.py:30: def RunEslintGlobal():
On 2016/09/23 at 10:17:49, nednguyen wrote:
> Can you merge this to RunEslintOnDir(dir_path), the DIRECTORIES_TO_LINT should
be moved to the script size & not be hardcoded in the module

Done.

https://codereview.chromium.org/2361623007/diff/20001/common/node_runner/bin/...
File common/node_runner/bin/test_node_for_smoke (right):

https://codereview.chromium.org/2361623007/diff/20001/common/node_runner/bin/...
common/node_runner/bin/test_node_for_smoke:1: #!/usr/bin/env python
On 2016/09/23 at 10:17:49, nednguyen wrote:
> We can now remove this & change the catapult_build/build_steps.py to run the	
common/eslint/bin/run_eslint instead. 
> 
> Having a circular dependency between node_runner & eslint is bad

Good idea. Done.

https://codereview.chromium.org/2361623007/diff/80001/common/eslint/bin/run_e...
File common/eslint/bin/run_eslint (right):

https://codereview.chromium.org/2361623007/diff/80001/common/eslint/bin/run_e...
common/eslint/bin/run_eslint:30: if __name__ == '__main__':
Is there a good place to provide usage information on this?

[nednguyen, 2016-09-23 13:16:04]

https://codereview.chromium.org/2361623007/diff/80001/common/eslint/bin/run_e...
File common/eslint/bin/run_eslint (right):

https://codereview.chromium.org/2361623007/diff/80001/common/eslint/bin/run_e...
common/eslint/bin/run_eslint:30: if __name__ == '__main__':
On 2016/09/23 13:06:35, charliea wrote:
> Is there a good place to provide usage information on this?

Best is to use argparse & make commandline flag instead of manipulating sys.argv
:-)

[nednguyen, 2016-09-23 13:22:13]

https://codereview.chromium.org/2361623007/diff/80001/catapult_build/build_st...
File catapult_build/build_steps.py (right):

https://codereview.chromium.org/2361623007/diff/80001/catapult_build/build_st...
catapult_build/build_steps.py:91: 'additional_args': ['--help'],
Hmhh, the point of running this is we can make sure that we are able to execute
node_js across platforms hence once people update the node_js ref, we have more
guarantee that things won't break. Running --help seems to defeat that purpose.

[nednguyen, 2016-09-23 13:24:26]

https://codereview.chromium.org/2361623007/diff/80001/catapult_build/build_st...
File catapult_build/build_steps.py (right):

https://codereview.chromium.org/2361623007/diff/80001/catapult_build/build_st...
catapult_build/build_steps.py:91: 'additional_args': ['--help'],
On 2016/09/23 13:22:13, nednguyen wrote:
> Hmhh, the point of running this is we can make sure that we are able to
execute
> node_js across platforms hence once people update the node_js ref, we have
more
> guarantee that things won't break. Running --help seems to defeat that
purpose.

I think it's best to create a smoke unittest for eslint, s.t like
eslint_unittest that run lint on a test html file.

Then here, we run: common/eslint/bin/run_tests

[charliea (OOO until 10-5), 2016-10-05 16:07:17]

PTAL

[nednguyen, 2016-10-05 16:13:34]

lgtm with nits

https://codereview.chromium.org/2361623007/diff/120001/catapult_build/build_s...
File catapult_build/build_steps.py (right):

https://codereview.chromium.org/2361623007/diff/120001/catapult_build/build_s...
catapult_build/build_steps.py:89: 'name': 'eslint Smoke Tests',
nits: can you remove the "Smoke" part

https://codereview.chromium.org/2361623007/diff/120001/common/eslint/eslint/_...
File common/eslint/eslint/__init__.py (right):

https://codereview.chromium.org/2361623007/diff/120001/common/eslint/eslint/_...
common/eslint/eslint/__init__.py:37: os.path.join(py_utils.GetCatapultDir(),
'common', 'eslint', '.eslintrc'),
I would make the "config" path & rules configurable. So you add eslintrc_path &
rulesdir as parameters in RunEslintOnDirs & RunEslintOnFiles with values default
to these

https://codereview.chromium.org/2361623007/diff/120001/common/eslint/eslint/_...
common/eslint/eslint/__init__.py:54: def RunEslintOnFiles(filenames=None):
Why bother having a default value for filenames as it should throw exception in
the default case?

[charliea (OOO until 10-5), 2016-10-05 17:24:43]

https://codereview.chromium.org/2361623007/diff/120001/catapult_build/build_s...
File catapult_build/build_steps.py (right):

https://codereview.chromium.org/2361623007/diff/120001/catapult_build/build_s...
catapult_build/build_steps.py:89: 'name': 'eslint Smoke Tests',
On 2016/10/05 16:13:33, nednguyen wrote:
> nits: can you remove the "Smoke" part 

Done.

https://codereview.chromium.org/2361623007/diff/120001/common/eslint/eslint/_...
File common/eslint/eslint/__init__.py (right):

https://codereview.chromium.org/2361623007/diff/120001/common/eslint/eslint/_...
common/eslint/eslint/__init__.py:37: os.path.join(py_utils.GetCatapultDir(),
'common', 'eslint', '.eslintrc'),
On 2016/10/05 16:13:33, nednguyen wrote:
> I would make the "config" path & rules configurable. So you add eslintrc_path
&
> rulesdir as parameters in RunEslintOnDirs & RunEslintOnFiles with values
default
> to these

Done.

https://codereview.chromium.org/2361623007/diff/120001/common/eslint/eslint/_...
common/eslint/eslint/__init__.py:54: def RunEslintOnFiles(filenames=None):
On 2016/10/05 16:13:33, nednguyen wrote:
> Why bother having a default value for filenames as it should throw exception
in
> the default case?

Good point. I started raising an error when filenames is none or []. I also did
the same thing above for RunEslintOnDirs().

[charliea (OOO until 10-5), 2016-10-05 17:25:21]

The CQ bit was checked by charliea@chromium.org

[charliea (OOO until 10-5), 2016-10-05 17:25:21]

The patchset sent to the CQ was uploaded after l-g-t-m from nednguyen@google.com
Link to the patchset: https://codereview.chromium.org/2361623007/#ps160001
(title: "Synced to head")

[commit-bot: I haz the power, 2016-10-05 17:25:32]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-05 18:36:52]

Description was changed from

==========
Add a run_eslint wrapper script

In doing so, I also moved run_eslint to common/eslint.

With no arguments, this script lints all dashboard/ and tracing/ HTML
files. When passed arguments, it assumes those arguments are the files
to be linted.

BUG=catapult:#2810
==========

to

==========
Add a run_eslint wrapper script

In doing so, I also moved run_eslint to common/eslint.

With no arguments, this script lints all dashboard/ and tracing/ HTML
files. When passed arguments, it assumes those arguments are the files
to be linted.

BUG=catapult:#2810

Committed:
https://chromium.googlesource.com/external/github.com/catapult-project/catapu...
==========

[commit-bot: I haz the power, 2016-10-05 18:36:53]

Committed patchset #6 (id:160001) as
https://chromium.googlesource.com/external/github.com/catapult-project/catapu...