Skip to content

Added a github action to run the pre-commit hook and display a diff on failure#92

Open
hello-robot-shehab wants to merge 10 commits intomainfrom
feature/pre-commit
Open

Added a github action to run the pre-commit hook and display a diff on failure#92
hello-robot-shehab wants to merge 10 commits intomainfrom
feature/pre-commit

Conversation

@hello-robot-shehab
Copy link
Contributor

  • Also changed all the formatters to check and display a diff, instead of apply the fixes.
  • Added a Contributing section to the README to explain how to install the pre-commit hook.

@hello-robot-shehab
Copy link
Contributor Author

hello-robot-shehab commented Jun 12, 2025

Pre-commit hook will pass when #91 is merged into this PR. Thanks!

Edit: we're removing formatters and linting checks that affect styling.

@hello-robot-shehab
Copy link
Contributor Author

Merging this PR will always cause the github action to fail, unless we add feature/pre-commit...linting/isort-pass, is this okay with you? We could also just nuke the pre-commit hook entirely if you don't think it's worthwhile, thanks!

@hello-binit
Copy link
Contributor

I think we can remove isort and keep the rest. My understanding of isort was that it would remove unnecessary imports only. From looking at its docs, it looks like the formatter is tightly integrated, and unnecessary imports are just one of many things it checks for.

@hello-robot-shehab
Copy link
Contributor Author

I think we can remove isort and keep the rest. My understanding of isort was that it would remove unnecessary imports only. From looking at its docs, it looks like the formatter is tightly integrated, and unnecessary imports are just one of many things it checks for.

Done, thanks!

@hello-binit
Copy link
Contributor

Looks like mypy isn't running. Any ideas why?

mypy.................................................(no files to check)Skipped

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.

2 participants