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

Refactor check_banned_imports.py to allow Python 3 files to import subprocess #7706

Merged

Conversation

Eric-Arellano
Copy link
Contributor

@Eric-Arellano Eric-Arellano commented May 11, 2019

Problem

Once we drop Python 2, we will start directly importing the stdlib module for subprocess and will stop using subprocess32 and pants.util.process_handler.subprocess. However, our check_banned_imports.py would not allow this to happen.

At the same time, we would like to keep checking that our Python 2 files are valid, so we don't want to remove the check outright.

Solution

Extract out a filter_files() function that will remove any files without the given snippet. This allows us to identify and save in memory all of our Python 2 files, then to call check_banned_import on only those files.

We check for the presence of from __future__ imports, which are only used in our codebase for Python 2 compatibility.

Result

We can now import subprocess in Python 3-only files.

There is a 1-second slowdown in the performance of that script. After we finish dropping Python 2, we could reclaim this second by removing both the filter to get Python 2 only files and removing the subprocess check entirely.

Copy link
Contributor

@blorente blorente left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Couple of nits, but looks great!

build-support/bin/check_banned_imports.py Outdated Show resolved Hide resolved
build-support/bin/check_banned_imports.py Show resolved Hide resolved
@Eric-Arellano Eric-Arellano merged commit b467188 into pantsbuild:master May 13, 2019
@Eric-Arellano Eric-Arellano deleted the allow-subprocess-import branch May 13, 2019 17:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants