Skip to content

Commit

Permalink
Allow repos to override khan-linter's linting on a per-language basis.
Browse files Browse the repository at this point in the history
Summary:
Khan-linter now looks for a config file in a repo, before deciding how
to lint any files in that repo.  If it finds one, it sees if the
config file has its own rules for how to lint that file.  If so, it
does those rules rather than its built-in ones.

This will let us migrate linters from khan-linter to webapp
piece-by-piece, without having to change khan-linter each time we do
so.  It also means we don't have to worry about forward- and
backward-compability as people change branches, since any new webapp
linters will be committed in the same commit as the config-file
change.

Issue: TODO

Test Plan:
I created this lintconfig.yaml in webapp:
```
khan-linter-overrides:
  yaml:
    - ["echo", "hi""]
```
and ran
   ka-lint lintconfig.yaml
and it emitted:
   hi lintconfig.yaml
like the new rule said!

I then ran `ka-lint main.py` and it did not print `hi`.

I then ran
   cd assignments; ka-lint ../lintconfig.yaml
and it said
   ka-lint lintconfig.yaml
again, because we run stuff from webapp-root.  That may yield
a bit of a surprise to folks that lint is being run in a different
directory than they're in, but hopefully it won't be too bad.

Reviewers: benkraft, aviddy

Reviewed By: benkraft

Subscribers: davidbraley, kevinb

Differential Revision: https://phabricator.khanacademy.org/D62448
  • Loading branch information
csilvers committed Apr 20, 2020
1 parent 91a60ac commit d8361cb
Show file tree
Hide file tree
Showing 2 changed files with 56 additions and 0 deletions.
28 changes: 28 additions & 0 deletions linters.py
Original file line number Diff line number Diff line change
Expand Up @@ -120,6 +120,34 @@ def report(self, lint):
lint_util.print_(lint)


class DelegatingLinter(Linter):
"""A linter that just calls out to a binary to do its actual work."""
def __init__(self, argv, cwd, logger):
"""argv is the command to run (as a lint). cwd is where to run it."""
super(DelegatingLinter, self).__init__(logger)
self._argv = argv
self._cwd = cwd

def process_files(self, files):
# We need to refer to all filenames relative to self._cwd.
files = [f if os.path.isabs(f) else os.path.relpath(f, self._cwd)
for f in files]

pipe = subprocess.Popen(
self._argv + files,
stdout=subprocess.PIPE,
stderr=subprocess.PIPE,
cwd=self._cwd)
stdout, stderr = pipe.communicate()

if stderr:
raise RuntimeError("Unexpected stderr from linter:\n%s" % stderr)
lines = stdout.rstrip('\n').splitlines()
for line in lines:
self.report(line)
return len(lines)


def _capture_stdout_of(fn, *args, **kwargs):
"""Call fn(*args, **kwargs) and return (fn_retval, fn_stdout_output_fp)."""
try:
Expand Down
28 changes: 28 additions & 0 deletions runlint.py
Original file line number Diff line number Diff line change
Expand Up @@ -615,6 +615,23 @@ def _find_base_config(file_to_lint, config_filename):
return None


_REPO_CONFIG_CACHE = {}


def _find_repo_config(file_to_lint):
"""Look for lintconfig.yaml for this file, and return it if found."""
repo_config_file = _find_base_config(file_to_lint, 'lintconfig.yaml')
if not repo_config_file:
return (None, {})

if repo_config_file not in _REPO_CONFIG_CACHE:
with open(repo_config_file) as f:
_REPO_CONFIG_CACHE[repo_config_file] = linters.yaml.safe_load(f)

return (os.path.dirname(repo_config_file),
_REPO_CONFIG_CACHE[repo_config_file])


def _get_linters_for_file(file_to_lint, lang, propose_arc_fixes):
"""Return the linters we wish to run for this file.
Expand Down Expand Up @@ -671,6 +688,17 @@ def _get_linters_for_file(file_to_lint, lang, propose_arc_fixes):

file_lang = _lang(file_to_lint, lang)

# We allow a repository to override linters for a particular
# language. See if our repository does so.
(repo_config_dir, repo_config) = _find_repo_config(file_to_lint)
if repo_config:
cmds = repo_config.get('khan-linter-overrides', {}).get(file_lang)
if cmds is not None:
return [
linters.DelegatingLinter(cmd, repo_config_dir, _get_logger())
for cmd in cmds
]

# We support multiple configuration files for eslint and our graphql
# schema linter, , which allows runlint to run against subrepos with
# different configurations.
Expand Down

0 comments on commit d8361cb

Please sign in to comment.