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

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

Merged
merged 3 commits into from
Apr 8, 2024

Conversation

dheavy
Copy link
Contributor

@dheavy dheavy commented Apr 1, 2024

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

  • Update pre-commit entry to instead run a python script (for platform agnosticism) mitigating the entry.
  • Update contribution guidelines on this specific topic.

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

  • macOS
  • Windows 10
  • Ubuntu

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.

@dheavy dheavy marked this pull request as draft April 1, 2024 17:18
@dheavy dheavy changed the title Fix pytest call from pre-commit hook [WIP] Fix pytest call from pre-commit hook Apr 1, 2024
@dheavy dheavy marked this pull request as ready for review April 1, 2024 17:24
@dheavy dheavy changed the title [WIP] Fix pytest call from pre-commit hook Fix pytest call from pre-commit hook Apr 1, 2024
@tyfiero
Copy link
Collaborator

tyfiero commented Apr 2, 2024

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
Copy link
Contributor Author

dheavy commented Apr 3, 2024

@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?

@dheavy dheavy mentioned this pull request Apr 3, 2024
3 tasks
@tyfiero
Copy link
Collaborator

tyfiero commented Apr 4, 2024

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
Copy link
Contributor Author

dheavy commented Apr 5, 2024

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 dheavy marked this pull request as draft April 7, 2024 13:03
@dheavy dheavy changed the title Fix pytest call from pre-commit hook [WIP] Fix pytest call from pre-commit hook Apr 7, 2024
@dheavy dheavy changed the title [WIP] Fix pytest call from pre-commit hook [WIP] Fix pytest call from pre-commit hook, change linter to black Apr 7, 2024
@dheavy dheavy marked this pull request as ready for review April 7, 2024 13:20
@dheavy dheavy changed the title [WIP] Fix pytest call from pre-commit hook, change linter to black Fix pytest call from pre-commit hook, change linter to black Apr 7, 2024
@dheavy
Copy link
Contributor Author

dheavy commented Apr 7, 2024

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

@tyfiero tyfiero merged commit 48c5d89 into OpenInterpreter:main Apr 8, 2024
1 check passed
@tyfiero
Copy link
Collaborator

tyfiero commented Apr 8, 2024

Done deal! Thanks again

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Pre-commit fails at "pytest" step
2 participants