Utility to detect changed python files #28
Conversation
JasonMWhite
force-pushed
the
git_function
branch
from
cb8c239 to
90a7221
Compare
| main_repo.index.commit("adding python files") | ||
|
|
||
| changed_files = git_utils.changed_python_files_in_tree(main_repo.working_dir) | ||
| assert sorted(changed_files) == ['program', 'program.py'] |
There was a problem hiding this comment.
Maybe you could get program and program.py from the test fixtures? E.g. a function that performs os.path.split(python_file)[-1]?
There was a problem hiding this comment.
Alternatively, changed_python_files could return absolute paths. I go back and forth on this.
There was a problem hiding this comment.
Absolute paths aren't as easy to debug when differences are found, but they are exact.
There was a problem hiding this comment.
I think paths relative to the changed_python_files_in_tree argument is the most consistent with stdlib etc.
If this were being wrapped in a command-line script the most appropriate output would also be relative to the CWD (off the top of my head this is the behaviour of most Unix tools).
shopify_python/git_utils.py
Outdated
| import os | ||
| import typing # pylint: disable=W0611 | ||
| from git import repo | ||
| from git.refs import head # pylint: disable=W0611 |
There was a problem hiding this comment.
We should get in the habit of disabling by name to assist reviewers (#32)
| # type: (str) -> bool | ||
| if path.endswith('.py'): | ||
| return True | ||
| else: |
There was a problem hiding this comment.
Should this be restricted to files without extensions?
There was a problem hiding this comment.
What about files with +x? Too specific?
| 'pylint==1.6.5', | ||
| 'six>=1.10.0', | ||
| 'typing>=3.5.3.0', | ||
| 'GitPython==2.1.1', |
There was a problem hiding this comment.
Should this go in extra_require['dev']? Same for pylint actually.
There was a problem hiding this comment.
If shopify_python.git_utils is part of our public API, then GitPython remains here. Likewise with pylint.
| @pytest.fixture | ||
| def python_file(main_repo): | ||
| # type: (repo.Repo) -> str | ||
| file_text = "import os\n" \ |
There was a problem hiding this comment.
https://google.github.io/styleguide/pyguide.html?showone=Line_length#Line_length
Do not use backslash line continuation.
Make use of Python's implicit line joining inside parentheses, brackets and braces. If necessary, you can add an extra pair of parentheses around an expression.
There was a problem hiding this comment.
Right. We need a linter rule for this, too. :)
JasonMWhite
force-pushed
the
git_function
branch
from
c3a5397 to
afd7047
Compare
| with open(path) as might_be_python: | ||
| line = might_be_python.readline() | ||
| return line.startswith('#!') and 'python' in line | ||
| except UnicodeDecodeError: |
There was a problem hiding this comment.
Is this restriction documented anywhere?
There was a problem hiding this comment.
There's a test for it - it's thrown whenever we try to open a binary file.
4 participants
We commonly use an untested script, copied from one repo to another, as a git push hook to automatically autopep, lint, and (sometimes) type-check on the modified files prior to pushing to GitHub. Let's pull that into tested utilities.
First step is a utility to compare what is being pushed to what is on
origin/master. This utility function uses what has been committed in the current branch (not added to the index, or untracked files), and compares it toorigin/master. Any files that have been added or modified will be returned, provided they are also Python files or scripts.There are a couple of unrelated changes that are also in other PRs (.gitignoreandpylintrcfile changes). I'll rebase this PR after those merge.@cfournie @erikwright @honkfestival ?