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

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

Merged
merged 2 commits into from
Nov 9, 2023

Conversation

javulticat
Copy link
Member

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.

@javulticat javulticat self-assigned this Nov 7, 2023
@coveralls
Copy link

coveralls commented Nov 7, 2023

Coverage Status

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

@monkut
Copy link
Collaborator

monkut commented Nov 9, 2023

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

#1209

@javulticat javulticat merged commit 10316af into master Nov 9, 2023
12 checks passed
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.

3 participants