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 and modernize `check_header.py` to use Python 3 #7635

Merged
merged 11 commits into from May 13, 2019

Conversation

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

commented Apr 28, 2019

Makes the following improvements:

  • Uses argparse instead of reading from sys.argv, which brings these benefits:
    • No longer reads from stdin to get added files. Instead, takes an optional arg --files-added. This makes the script more reliable and simplifies the callers of the script (i.e. the tests and pre-commit).
    • No longer calls the script on the file itself, which was being done because sys.argv[0] is the name of the script.
    • check_header.py --help now works.
  • Rewrites check_header() to not use a buffer.
    • The original motivation for a buffer was for performance, to avoid materializing the content of every checked file.
    • The downside is that buffer code is harder to read because it is far more low-level and imperative and requires keeping track of where the buffer is currently at.
    • Instead, can get same performance with better readability through a list comprehension that uses f.readline() 7 times.
  • Inverts several conditionals in check_header.py for less nesting.
  • Renames check_header_helper.py to check_header.py. This script is now the top level script, and not a helper.
  • Replaces the error message failed to parse header at all with missing the expected header and the header does not match the expected header.
    • failed to parse header at all is not very descriptive, and suggests their was an issue with IO.
  • Adds type hints for better readability and less bugs.
  • Removes unnecessary __future__ imports and file encoding header.
  • Use textwrap.dedent() where possible for better indentation.

Eric-Arellano added some commits Apr 28, 2019

Use argparse instead of sys.argv
Greatly simplifies the script's interface so that it no longer depends on stdin and does not need an env var override, which was surprising.

@Eric-Arellano Eric-Arellano requested a review from cosmicexplorer Apr 28, 2019

@jsirois
Copy link
Member

left a comment

I think you might not be aware of #7515
What do you think about instead generating a net-red PR that kills all this and switches us to running ./pants --v2 --no-v1 validate [targets]?

@Eric-Arellano

This comment has been minimized.

Copy link
Contributor Author

commented Apr 29, 2019

I think you might not be aware of #7515

Totally forgot that's a thing.

What do you think about instead generating a net-red PR that kills all this and switches us to running ./pants --v2 --no-v1 validate [targets]?

I'd love to see it happen. I unfortunately don't think I'll be able to be the one to do it. I start work with Twitter tomorrow and I'll consequently have less time for "fun" refactors like this / will be much more focused in what my PRs involve.

--

Ftr, my main motivation here is that we soon will need to allow the header to be either Py2 style (current) or Py3 style (no __future__ or encoding line). I was looking at making that change now, and got overwhelmed by the file so ended up refactoring it. So, this is sort of pre-work for dropping Py2.

@stuhood stuhood requested a review from benjyw Apr 29, 2019

@stuhood

This comment has been minimized.

Copy link
Member

commented Apr 29, 2019

@benjyw : Do you have time to take #7515 across the finish line?

@Eric-Arellano

This comment has been minimized.

Copy link
Contributor Author

commented May 9, 2019

Bump on this thread.

1.17.0.dev0 is now upon us, so we can very soon start removing Py2. Before doing that, we need to update check_header.py to allow the Py3 header (no __future__ or # encoding lines). This is pre-work for adding that ability.

Unless someone has time to instead use #7515, or Twitter would like me to spend time on that approach over V2 work, I encourage us to use this PR for now.

@stuhood
Copy link
Member

left a comment

Given that we're not ready to use #7515, let's move forward with this. Thanks!

@Eric-Arellano Eric-Arellano merged commit a59bd05 into pantsbuild:master May 13, 2019

1 check passed

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

@Eric-Arellano Eric-Arellano deleted the Eric-Arellano:better-header-check branch May 13, 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.