OpenInterpreter / 01

The open-source language model computer
http://openinterpreter.com/01
GNU Affero General Public License v3.0
4.76k stars 470 forks source link

Fix pytest call from pre-commit hook, change linter to black #213

Closed dheavy closed 3 months ago

dheavy commented 3 months ago

Closes #210.

Problem

The pytest step in the pre-commit hook only works when ran from the software directory. Meaning if you commit from anywhere else (e.g. root directory) it will fail, preventing you from committing. This incentivizes using --no-verify to push code to a PR, bypassing code checks for quality when the pre-commit hook is actually installed.

Also, contribution guidelines weren't up-to-date regarding the pre-commit hook. They also failed to mention the subtle issue of environment specific pre-commit install, which may lead to confusing state when running pytest if it is not installed globally.

Solution

How to test

  1. Change into the 01/software directory
  2. Activate poetry shell
  3. Change into the root directory (cd ..)
  4. Run pre-commit run pytest --all-files

Alternatively for step 4, create a file (touch foo) and commit it (git add foo && git commit -m "foo"). Then git reset HEAD~1 to cancel the commit and rm foo to remove the file.

Tested on

Discussion

A specific tweak is necessary for Windows (version 10 at least) PowerShell, otherwise the pytest step will stale and fail silently (pre-commit in verbose mode allowed to catch the issue.

Another notable change stemming from the PR conversation is the confusion over the linter (was announced as black in the contribution docs, but ruff was installed instead). For consistency with interpreter, it was changed to black.

tyfiero commented 3 months ago

Is there a reason why you want to switch to Ruff? If all is equivalent I think we want to stick with black for the time being

dheavy commented 3 months ago

@tyfiero actually I did not switch. The default linter used in 01 was already ruff, contrary to open-interpreter, making the documentation confusing. Sorry for the confusion, how do you want to proceed?

tyfiero commented 3 months ago

lol of course. My bad. I guess it probably should be black to be consistent with the open-interpreter repo, what do you think?

dheavy commented 3 months ago

Agreed, I have no preference but consistency. I will move the PR to Draft and update it to black today or this weekend. Thanks for your time @tyfiero!

dheavy commented 3 months ago

@tyfiero updateded linter from ruff to black, ready for another review.

tyfiero commented 3 months ago

Done deal! Thanks again