Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Convert `check_banned_imports.sh` to Python to workaround Bash array issues and for less duplication #7702

Merged
merged 2 commits into from May 10, 2019

Conversation

Projects
None yet
2 participants
@Eric-Arellano
Copy link
Contributor

commented May 10, 2019

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.

@Eric-Arellano Eric-Arellano changed the title Convert `check_banned_imports.sh` to Python for less duplication and better readability Convert `check_banned_imports.sh` to Python to workaround Bash array issues and for less duplication May 10, 2019

@Eric-Arellano Eric-Arellano merged commit 762c03a into pantsbuild:master May 10, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@Eric-Arellano Eric-Arellano deleted the Eric-Arellano:banned-imports-py branch May 10, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.