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

Add whitelist of folders that must remain Python 2 compatible #7941

Merged
merged 6 commits into from Jun 24, 2019

Conversation

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

commented Jun 23, 2019

Three of our folders may be run with Python 2, so they must keep their compatibility, e.g. from __future__ imports.

If this impacted more code, we would want to create (restore) formal linters to enforce the compatibility. However, because the code is so small and CI will fail upon dropping Py2 support, we simply add comments to the files explaining this expectation and modify check_headers.py to allow these files to not use Py3 headers.

In the process, we refactor check_headers.py to use the pathlib module for much nicer path handling.

@Eric-Arellano Eric-Arellano requested a review from jsirois Jun 23, 2019

@Eric-Arellano
Copy link
Contributor Author

left a comment

Each commit may be reviewed independently.

# We grab an extra line in case there is a shebang.
lines = [f.readline() for _ in range(0, EXPECTED_NUM_LINES + 1)]
except IOError as e:
raise HeaderCheckFailure(f"{filename}: error while reading input ({e})")
except OSError as e:

This comment has been minimized.

Copy link
@Eric-Arellano

Eric-Arellano Jun 23, 2019

Author Contributor

Python 3 unified IOError into OSError: https://docs.python.org/3/library/exceptions.html#OSError.

# Copyright 2018 Pants project contributors (see CONTRIBUTORS.md).
# Licensed under the Apache License, Version 2.0 (see LICENSE).

from __future__ import absolute_import, division, print_function, unicode_literals

# NB: This file must keep Python 2 support because it is a resource that may be ran with Python 2.

This comment has been minimized.

Copy link
@jsirois

jsirois Jun 24, 2019

Member

The PR description has this usage as well and it seems off. I think 'may be run' is what's needed.

This comment has been minimized.

Copy link
@Eric-Arellano

Eric-Arellano Jun 24, 2019

Author Contributor

I think I agree. No matter how many times I read grammar girls, I will still be eternally confused by 'run' vs 'ran' :/

This comment has been minimized.

Copy link
@jsirois

jsirois Jun 24, 2019

Member

I don't know grammar, but it just struck me as mixing time zones. The 'may be' bit implies future. The 'ran' bit is past tense.

CURRENT_YEAR = str(datetime.datetime.now().year)
CURRENT_CENTURY_REGEX = re.compile(r'20\d\d')

PY2_DIRECTORIES = {

This comment has been minimized.

Copy link
@jsirois

jsirois Jun 24, 2019

Member

This all looks good but if I haven't pointed it out already, we already have a rule that does all this introduced in #7515. It already has enough flexibility to apply header checks to subsets of paths although it uses whitelists and could stand support for blacklists to make path specification more concise and less brittle for cases like these. We use the rule in the toolchain codebase; so I've filed #7942 to add blacklists and circling back to replace this hand-rolled check with use of the rule.

This comment has been minimized.

Copy link
@Eric-Arellano

Eric-Arellano Jun 24, 2019

Author Contributor

Ack. Always a fan of less code.

@Eric-Arellano
Copy link
Contributor Author

left a comment

Going to fix comments to use proper grammar and merge without another CI run.

CURRENT_YEAR = str(datetime.datetime.now().year)
CURRENT_CENTURY_REGEX = re.compile(r'20\d\d')

PY2_DIRECTORIES = {

This comment has been minimized.

Copy link
@Eric-Arellano

Eric-Arellano Jun 24, 2019

Author Contributor

Ack. Always a fan of less code.

# Copyright 2018 Pants project contributors (see CONTRIBUTORS.md).
# Licensed under the Apache License, Version 2.0 (see LICENSE).

from __future__ import absolute_import, division, print_function, unicode_literals

# NB: This file must keep Python 2 support because it is a resource that may be ran with Python 2.

This comment has been minimized.

Copy link
@Eric-Arellano

Eric-Arellano Jun 24, 2019

Author Contributor

I think I agree. No matter how many times I read grammar girls, I will still be eternally confused by 'run' vs 'ran' :/

@Eric-Arellano Eric-Arellano merged commit 4514aa5 into pantsbuild:master Jun 24, 2019

1 check failed

continuous-integration/travis-ci/pr The Travis CI build could not complete due to an error
Details

@Eric-Arellano Eric-Arellano deleted the Eric-Arellano:whitelist-py2-headers branch Jun 24, 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.