public
/
pyceo
27
1
Fork 3
Code Issues 25 Pull Requests 5 Projects Releases 1 Wiki Activity

Add linting pre-commit hook and hook install script #86

Merged
merenber merged 6 commits from feature-80 into master 2023-05-29 00:14:06 -04:00
Conversation 1 Commits 6 Files Changed 3 +45
2 changed files with 45 additions and 0 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files
Showing only changes of commit da51c4309c - Show all commits

41
.githooks/pre-commit Executable file
View File

@ -0,0 +1,41 @@
#!/bin/sh
STAGED_FILES=$(git diff --cached --name-only --diff-filter=ACM | grep ".py\{0,1\}$")
j24chung marked this conversation as resolved Outdated
merenber commented 2023-05-27 21:40:11 -04:00
Outdated
Review
Copy Link

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$'.

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$'`.
merenber commented 2023-05-27 21:43:15 -04:00
Outdated
Review
Copy Link

Actually, we can get rid of the STAGED_FILES variable altogether if we just invoke flake8 with no arguments - see comment below.

Actually, we can get rid of the STAGED_FILES variable altogether if we just invoke flake8 with no arguments - see comment below.
if [[ "$STAGED_FILES" = "" ]]; then
exit 0
fi
PASS=true
j24chung marked this conversation as resolved Outdated
merenber commented 2023-05-27 21:40:27 -04:00
Outdated
Review
Copy Link

We use the 3.9-bullseye image now.

We use the `3.9-bullseye` image now.
# Check for flake8
j24chung marked this conversation as resolved Outdated
merenber commented 2022-11-26 23:16:47 -05:00
Outdated
Review
Copy Link

I 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 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.
r345liu commented 2022-11-27 10:25:39 -05:00
Outdated
Review
Copy Link

I think we can assume git commit would be running inside virtual env, plus venv directory can be located elsewhere too

I think we can assume git commit would be running inside virtual env, plus venv directory can be located elsewhere too
j24chung commented 2023-02-04 00:08:57 -05:00
Outdated
Review
Copy Link

Do you mind clarifying this flake8 script you're referring to? @merenber

Do you mind clarifying this flake8 script you're referring to? @merenber
merenber commented 2023-02-04 00:38:39 -05:00
Outdated
Review
Copy Link

It's in dev-requirements.txt. On my machine, I run flake8 from inside one of the Docker containers, in the activated venv, but I run git commit outside 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.

It's in dev-requirements.txt. On my machine, I run `flake8` from inside one of the Docker containers, in the activated venv, but I run `git commit` outside 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.
j24chung commented 2023-05-27 15:19:50 -04:00
Outdated
Review
Copy Link

It's in dev-requirements.txt. On my machine, I run flake8 from inside one of the Docker containers, in the activated venv, but I run git commit outside 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 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.

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.

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.

> It's in dev-requirements.txt. On my machine, I run `flake8` from inside one of the Docker containers, in the activated venv, but I run `git commit` outside 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 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. > 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. 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.
j24chung commented 2023-05-27 16:44:46 -04:00
Outdated
Review
Copy Link

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

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
which flake8 &> /dev/null
if [[ "$?" == 1 ]]; then
echo -e "\t\033[41mPlease install flake8\033[0m"
exit 1
fi
echo -e "\nLinting Python with Flake8:\n"
for FILE in $STAGED_FILES
do
flake8 "$FILE"
j24chung marked this conversation as resolved Outdated
merenber commented 2022-11-26 23:12:11 -05:00
Outdated
Review
Copy Link

Is there a reason why we need to invoke flake8 on each file separately? If you just run flake8 once from the root directory, it'll lint all of the source files at once.

Is there a reason why we need to invoke flake8 on each file separately? If you just run `flake8` once from the root directory, it'll lint all of the source files at once.
j24chung commented 2023-02-04 00:10:00 -05:00
Outdated
Review
Copy Link

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.

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.
merenber commented 2023-02-04 00:41:35 -05:00
Outdated
Review
Copy Link

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.

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.
j24chung commented 2023-05-27 15:41:10 -04:00
Outdated
Review
Copy Link

At the end of the STAGED_FILES variable declaration it has grep ".py\{0,1\}$" so it should only grab python files

At the end of the STAGED_FILES variable declaration it has `grep ".py\{0,1\}$"` so it should only grab python files
if [[ "$?" == 0 ]]; then
echo -e "\t\033[32mPassed: $FILE\033[0m"
j24chung marked this conversation as resolved Outdated
merenber commented 2022-11-26 23:15:05 -05:00
Outdated
Review
Copy Link

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.

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.
j24chung commented 2023-05-27 15:41:45 -04:00
Outdated
Review
Copy Link

Fair. Will address it rn.

Fair. Will address it rn.
else
echo -e "\t\033[41mFailed: $FILE\033[0m"
PASS=false
fi
done
echo -e "\nPython linting complete!\n"
if ! $PASS; then
echo -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"
j24chung marked this conversation as resolved Outdated
merenber commented 2022-11-26 23:13:12 -05:00
Outdated
Review
Copy Link

Maybe we should change "ESLint" to "flake8"?

Maybe we should change "ESLint" to "flake8"?
j24chung commented 2023-02-04 00:10:20 -05:00
Outdated
Review
Copy Link

Haha oops. Need to fix my typo

Haha oops. Need to fix my typo
exit 1
else
echo -e "\033[42mCOMMIT SUCCEEDED\033[0m\n"
fi
exit $?

4
git-hook-install.sh Executable file
View File

@ -0,0 +1,4 @@
#!/bin/sh
# Install pre-configured git hooks
git config --local core.hooksPath .githooks/