Skip to content

Commit

Permalink
Use local eslint executable if available
Browse files Browse the repository at this point in the history
Summary:
This diff is intended to make a little progress on [[ https://docs.google.com/document/d/12hbgMCT6c5-fLZ7_fpUQWF8GkuCh-N-Kl4D7cruYMS4/edit#heading=h.zcx77itbdtis | ADR #273 ]].

In D62269 I updated webapp to include all necessary eslint plugins
and verified that the local eslint executable can be run with the
same results as ka-lint which until this diff uses the eslint executable
in this repo.

This diff updates khan-linter to use the local eslint executable in
the repo of the file being linted if it exists.  If it doesn't it falls
back to the eslint executable in the khan-linter repo.

Issue: none

Test Plan:
- in webapp (has its own eslint): ka-lint javascript/about-package
- in kpi-dashboard (no local eslint): ka-lint src/team

Reviewers: #frontend-infrastructure, benkraft, csilvers

Reviewed By: csilvers

Subscribers: csilvers

Differential Revision: https://phabricator.khanacademy.org/D62270
  • Loading branch information
kevinb-khan committed Apr 22, 2020
1 parent b2bf12e commit af4f8c6
Show file tree
Hide file tree
Showing 2 changed files with 28 additions and 17 deletions.
29 changes: 17 additions & 12 deletions linters.py
Original file line number Diff line number Diff line change
Expand Up @@ -392,10 +392,19 @@ class Eslint(Linter):
Arguments:
config_path: the path of the eslintrc file
logger: logger
[exec_path]: the path of the eslint executable, uses khan-linter's
copy of eslint if none is specified.
[propose_arc_fixes]: whether or not to propose fixes to arc, defaults
to False.
"""
def __init__(self, config_path, logger, propose_arc_fixes=False):
def __init__(self, config_path, logger,
exec_path=None,
propose_arc_fixes=False):
super(Eslint, self).__init__(logger=logger)
self._config_path = config_path
self._exec_path = (
exec_path or os.path.join(_CWD, 'node_modules', '.bin', 'eslint'))
self._propose_arc_fixes = propose_arc_fixes

def _maybe_add_arc_fix(self, lintline, bad_line):
Expand Down Expand Up @@ -578,17 +587,11 @@ def process(self, f, contents_of_f, eslint_lines):

def _run_eslint(self, files):
"""Run eslint on the given files and returns stdout, sans header."""
exec_path = os.path.join(_CWD, 'node_modules', '.bin', 'eslint')
reporter_path = os.path.join(_CWD, 'eslint_reporter.js')
assert os.path.isfile(exec_path), (
"Vendoring error: eslint is missing from '%s'" % exec_path)

# TODO(csilvers): split out files based on whether they're intended
# for node.js or not, and use eslintrc.node for the node.js files.
# Two ways to tell:
# 1) shebang line at the top of the file
# 2) '"use strict";' in the file somewhere
subprocess_args = [exec_path, '--config', self._config_path,
assert os.path.isfile(self._exec_path), (
"Error: eslint is missing from '%s'" % self._exec_path)

subprocess_args = [self._exec_path, '--config', self._config_path,
'-f', reporter_path, '--no-color'] + files

env = os.environ.copy()
Expand Down Expand Up @@ -617,7 +620,9 @@ def _run_eslint(self, files):

stdout, stderr = stdout.decode('utf-8'), stderr.decode('utf-8')

if stderr:
# eslint prints out a warning about browserslist to stderr even
# though it's just a warning.
if stderr and not stderr.startswith("Browserslist:"):
raise RuntimeError(
"Unexpected stderr from linter (exited %s):\n%s\nstdout:\n%s"
% (process.returncode, stderr, stdout))
Expand Down
16 changes: 11 additions & 5 deletions runlint.py
Original file line number Diff line number Diff line change
Expand Up @@ -709,23 +709,29 @@ def _get_linters_for_file(file_to_lint, lang, propose_arc_fixes):
return [_get_delegating_linter(cmd, repo_config_dir)
for cmd in cmds]

# We support multiple configuration files for eslint and our graphql
# schema linter, , which allows runlint to run against subrepos with
# We support multiple eslint configuration files as well as eslint
# executables which allows runlint to run against subrepos with
# different configurations.
if file_lang == 'javascript':
# If eslint_config is None, linters.Eslint will default to the
# configuration in the khan-linter repo.
eslint_config = _find_base_config(file_to_lint, '.eslintrc')
if eslint_config:
cache_key = "js-%s" % eslint_config
# The same is true for the eslint executable.
exec_path = _find_base_config(file_to_lint, 'node_modules/.bin/eslint')
if eslint_config or exec_path:
cache_key = "js-%s-%s" % (eslint_config, exec_path)
if cache_key not in _LINTERS_BY_LANG:
# Use the javascript linters, but replace the eslint
# linter with one that uses the config for our repo.
_LINTERS_BY_LANG[cache_key] = list(
_LINTERS_BY_LANG['javascript'])
_LINTERS_BY_LANG[cache_key][0] = (
linters.Eslint(eslint_config, _get_logger(),
propose_arc_fixes))
exec_path, propose_arc_fixes))
return _LINTERS_BY_LANG[cache_key]

# We also support multiple configuration files for our graphql schema
# linter for same reason.
if file_lang == 'sdl':
schema_config = _find_base_config(
file_to_lint, '.graphql-schema-linterrc')
Expand Down

0 comments on commit af4f8c6

Please sign in to comment.