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

Proposed addition of a base pre-commit hook using pre-commit framework #290

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

00willo
Copy link
Contributor

@00willo 00willo commented Apr 10, 2022

I'm proposing that pre-commit hooks be enabled by the pre-commit framework. I thought we could try it here to work out the agreed general style. Potentially it could be rolled out across the other repos.

I've added explicit commits for each fix introduced by the pre-commit. So if any are deemed not required or problematic they can be reverted in this PR.

There's definitely some opinionated parts to this, and these may not be liked based on personal styles, but they offer the following general benefits:

  • minimising diffs
  • reduce change of merge conflicts

I've staged some additional lint and formatting pre-commit checks to potentially add later.

To call out a couple of potential issues:

  • For formatting, black, it's just simple, it works, and most find it great for the most part. It standardises any style choices to what it enforces and help with the diffs and merge conflicts.
  • For import sorting, it may appear verbose, but again, comes down to all about minimising size of diffs and merge conflicts. It's not that bad, even if you're importing 100 things, the number of lines is easy to get over. A decent code editor will collapse them for you when required.

Initially add to this repository for developers to run locally. Then, it can be added to the CI/CD pipeline, to either block PR or release as appropriate.

@00willo
Copy link
Contributor Author

00willo commented Apr 10, 2022

Happy to break this up if required. 95% of this change is just trailing whitespace, end of line and and end of file newline fixes.

@00willo
Copy link
Contributor Author

00willo commented Apr 10, 2022

If there's agreed traction on this, I think it'd also be good to add additional information to a CONTRIBUTING.md on formatting, linting and style etc.

@frikky
Copy link
Member

frikky commented Apr 15, 2022

This seems mostly like a solution looking for a problem, but is something that may be needed later on, so I'll keep it open for now. The main problem becomes that adding this becomes yet another thing to manage, so I'll need someone else to do linting and verification long-term

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