Add linting pre-commit hook and hook install script #86
merenber
r345liu
Labels
No Label
priority
high
priority
low
priority
medium
priority
very high
BUG
Feature
High Priority
Low Priority
Medium Priority
No Milestone
No project
No Assignees
3 Participants
Notifications
Due Date
No due date set.
Dependencies
No dependencies set.
Reference: public/pyceo#86
Loading…
Reference in New Issue
No description provided.
Delete Branch "feature-80"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Might want to consider the case where file name can be white space. Just adding a
IFS='\n'before the for loop is probably enough.@ -0,0 +8,4 @@PASS=true# Check for flake8I suggest using the flake8 script which gets installed in venv/bin, so that the user doesn't have to install a ilbrary globally if they don't want to.
I think we can assume git commit would be running inside virtual env, plus venv directory can be located elsewhere too
Do you mind clarifying this flake8 script you're referring to? @merenber
It's in dev-requirements.txt. On my machine, I run
flake8from inside one of the Docker containers, in the activated venv, but I rungit commitoutside of the container, directly on the host. I cannot activate the venv outside of the container because my hosts's Python version is higher than that used in the Docker containers.I think it's fair to assume that people will want to use git outside of the container, since their SSH keys and global gitconfig are not present in the container. Instead of executing flake8 directly, I suggest we run it via docker-compose, e.g.
docker-compose exec phosphoric-acid flake8.I also suggest that we disable this hook by default, and require some kind of explicit user action to enable it. Not everyone will want to run a potentially time-consuming lint on every commit. I usually only run flake8 right before I push.
I agree and I also use Docker for development myself. But since README also has instructions for setting up a VM, would it be necessary for the script to suppor that? Since the script isn't enabled by default, then maybe it's okay to not support it since I believe most people would use the docker setup instead.
No problem. The script is currently a git hook script ran before each commit but I could make it a script ran ony before each push. The git hooks also require running the "git-hook-install.sh" script before it's enabled.
Update: The latest commit runs flake8 through docker but since it's just code linting, it just starts a python docker image and uses flake8 installed in the venv location created when setting up the docker setup. This should be good to close
@ -0,0 +19,4 @@for FILE in $STAGED_FILESdoflake8 "$FILE"Is there a reason why we need to invoke flake8 on each file separately? If you just run
flake8once from the root directory, it'll lint all of the source files at once.We are just linting all the files that are being committed. Technically, all files should be free of linting issues but I've just decided to only apply linting on the files that need to be changed.
What if one of the staged files is not a Python file? flake8 will exit with an error code if you run it directly on a non-Python file.
At the end of the STAGED_FILES variable declaration it has
grep ".py\{0,1\}$"so it should only grab python files@ -0,0 +22,4 @@flake8 "$FILE"if [[ "$?" == 0 ]]; thenecho -e "\t\033[32mPassed: $FILE\033[0m"flake8 provides its own diagnostic messages for each lint error, so I don't think we need to add an extra "Passed" or "Failed" message.
Fair. Will address it rn.
@ -0,0 +32,4 @@echo -e "\nPython linting complete!\n"if ! $PASS; thenecho -e "\033[41mCOMMIT FAILED:\033[0m Your commit contains files that should pass ESLint but do not. Please fix the ESLint errors and try again.\n"Maybe we should change "ESLint" to "flake8"?
Haha oops. Need to fix my typo
Add linting pre-commit hook and hook install scriptto WIP: Add linting pre-commit hook and hook install scriptWIP: Add linting pre-commit hook and hook install scriptto Add linting pre-commit hook and hook install scriptGood point. That'll be addressed
@ -0,0 +1,12 @@#!/bin/bashSTAGED_FILES=$(git diff --cached --name-only --diff-filter=ACM | grep ".py\{0,1\}$")That regex looks wrong to me - that will also match files which end with ".p" as well as "xp" (since the "." is a wildcard). I think maybe you want e.g.
grep '\.py$'.Actually, we can get rid of the STAGED_FILES variable altogether if we just invoke flake8 with no arguments - see comment below.
@ -0,0 +6,4 @@exit 0fidocker run --rm -v "$PWD:$PWD:z" -w "$PWD" -e "STAGED_FILES=$STAGED_FILES" python:3.7-buster sh -c './lint-docker.sh "$STAGED_FILES"'We use the
3.9-bullseyeimage now.@ -0,0 +10,4 @@PASS=truewhile IFS= read -r FILE; doI think this was mentioned before but in case it wasn't - why are we linting each file individually? flake8 can just be run with no arguments from the root directory, and it will be faster than creating a new process for each file.
My oriignal thought before was we'd just lint the files that's being modified but rethinking about it, there shouldn't it be a problem with just relinting everything which is easier