search

zappa / Zappa

Serverless Python
https://zappa.ws/zappa
MIT License
3.35k stars 362 forks source link

Configures 'pre-commit' hooks for Zappa's development tools #1284

Closed javulticat closed 1 year ago

javulticat commented 1 year ago

Adds pre-commit hooks for all of the major linting/formatting tools currently used in Zappa development.

Two choices I intentionally made to keep the scope of this PR small, low-risk, and easy to review:

  1. I specifically tried to keep the behavior of the pre-commit hooks to remain as close as possible to the currently defined behavior of the tools in our MAKEFILE, even though, in many cases, that behavior is sub-optimal, or even, in some cases, entirely useless (for example, our MAKEFILE has flake8 currently running with the --exit-zero arg always enabled, meaning that our CI always thinks it succeeds, despite the fact that flake8 does, in fact, not actually ever succeed when currently run on our repo and in fact, ends up finding a number of errors that this MAKEFILE configuration has hidden for... likely years at this point). I plan on properly configuring our dev tools to actually provide the value they are meant to in follow up PRs.
  2. I also specifically decided to not make any changes to the actual Python code powering Zappa in this PR. The only changes this PR makes to .py files are adding comments that instruct mypy to ignore certain lines that don't currently pass mypy's checks. Again, modernizing the codebase to better conform to all of the best practices imposed by tools like flake8 and mypy can wind up being massive undertakings on big, old, largely untyped codebases like ours that have seemingly never prioritized best practices too highly... So, I wanted to leave any code changes for the sake of pleasing our linting tools (and, thereby hopefully making our code much more readable and easy to follow) to be done via separate PR(s) as well so this one could be safely merged and begin making our contributors' lives easier sooner rather than later.

For more, see Issue #1282, which will be resolved by this PR.

coveralls commented 1 year ago

Coverage Status

coverage: 74.736%. remained the same when pulling 0a2d6f729316ba392a1f0ee61de9300e8bc40a99 on jav/pre-commit into 35af3cd7d1e76af902b9e22b9357d0c4b50e4655 on master.

monkut commented 1 year ago

@gaborschulz introduced pre-commit in the pull request below. It has a conflict and hasn't been merged in yet.

This looks good, I'm ok with getting this in, and the requesting conflict resolution when this is in.

https://github.com/zappa/Zappa/pull/1209