Skip to content

Commit

Permalink
Convert check_banned_imports.sh to Python to workaround Bash array…
Browse files Browse the repository at this point in the history
… issues and for less duplication (#7702)

### Problem
The current bash script violates Shellcheck for not properly quoting a variable. However, fixing this breaks the behavior and there is no simple way to fix it by converting the result of `find` to an array. See #7698 (comment).

This is not the first time the bash script has been difficult to work with. For example, #6747 took 26 commits to land and several back-and-forth code snippets over Slack.

Finally, the bash script has much duplication in it.

### Solution
Rewrite to modern Python 3.

### Result
Bad imports will be caught the same as before.

To de-duplicate logic, the printed user message is a bit different, however. The failure message is also now outputted in red.
  • Loading branch information
Eric-Arellano authored May 10, 2019
1 parent c0bfc9e commit 762c03a
Show file tree
Hide file tree
Showing 4 changed files with 65 additions and 26 deletions.
6 changes: 6 additions & 0 deletions build-support/bin/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,12 @@ python_library(
compatibility = ['CPython>=3.6'],
)

python_binary(
name = 'check_banned_imports',
sources = 'check_banned_imports.py',
compatibility = ['CPython>=3.6'],
)

python_binary(
name = 'check_header_helper',
sources = 'check_header_helper.py',
Expand Down
58 changes: 58 additions & 0 deletions build-support/bin/check_banned_imports.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,58 @@
#!/usr/bin/env python3
# Copyright 2019 Pants project contributors (see CONTRIBUTORS.md).
# Licensed under the Apache License, Version 2.0 (see LICENSE).

import re
from glob import glob
from typing import List

from common import die


def main() -> None:
python_files = find_files(
"src", "tests", "pants-plugins", "examples", "contrib", extension=".py"
)
rust_files = find_files("src/rust/engine", extension=".rs")

check_banned_import(
python_files,
bad_import_regex=r"^import subprocess$",
correct_import_message="`from pants.util.process_handler import subprocess`"
)
check_banned_import(
python_files,
bad_import_regex=r"^import future.moves.collections|^from future.moves.collections import|^from future.moves import .*collections",
correct_import_message="`import collections` or `from pants.util.collections_abc_backport`"
)
check_banned_import(
rust_files,
bad_import_regex=r"^use std::sync::.*(Mutex|RwLock)",
correct_import_message="`parking_lot::(Mutex|RwLock)`"
)


def find_files(*directories: str, extension: str) -> List[str]:
return [
fp
for directory in directories
for fp in glob(f"{directory}/**/*{extension}", recursive=True)
]


def check_banned_import(files: List[str], *, bad_import_regex: str, correct_import_message: str) -> None:
regex = re.compile(bad_import_regex)
bad_files: List[str] = []
for fp in files:
with open(fp, 'r') as f:
if any(re.search(regex, line) for line in f.readlines()):
bad_files.append(fp)
if bad_files:
bad_files_str = "\n".join(bad_files)
die(
f"Found forbidden imports matching `{bad_import_regex}`. Instead, you should use "
f"{correct_import_message}. Bad files:\n{bad_files_str}")


if __name__ == "__main__":
main()
25 changes: 0 additions & 25 deletions build-support/bin/check_banned_imports.sh

This file was deleted.

2 changes: 1 addition & 1 deletion build-support/githooks/pre-commit
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,7 @@ printf "%s\n" "${ADDED_FILES[@]}" \
| ./build-support/bin/check_header_helper.py "${DIRS_TO_CHECK[@]}"

echo "* Checking for banned imports"
./build-support/bin/check_banned_imports.sh
./build-support/bin/check_banned_imports.py

echo "* Checking for bad shell patterns" && ./build-support/bin/check_shell.sh || exit 1

Expand Down

0 comments on commit 762c03a

Please sign in to comment.