-
Notifications
You must be signed in to change notification settings - Fork 6
Feature/add pre commit hook for linting #217
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
Conversation
app/requirements/local.txt
Outdated
|
|
||
| psycopg2-binary==2.9.3 | ||
|
|
||
| black==19.3b |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changing black version now will make a huge black diff
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, as we said in #215. Fine with me, if you merge this PR after the workshop :)
|
@Bachibouzouk what is the situation with this PR / issue? I think it would be nice to get the pre-commit hooks set up once before starting back up with development, then it is out of the way :) |
|
Thanks @paulapreuss for taclking this, I think it make sense to tackle precommit at this stage |
|
I think this PR needs rebase |
7685297 to
05111db
Compare
|
@Bachibouzouk I rebased this on main, added some hooks that were not listed but we are using in CP Nigeria and updated the |
ce85177 to
de0c562
Compare
| @@ -1,18 +1,21 @@ | |||
| django-extensions==3.0.9 | |||
| Django==4.2.4 | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here we might even take the opportunity to upgrade to the latest Django version, I would rather do this in a separate PR and thus only rename the file without changing the dependencies (keeping the changes though)
| python manage.py update_assettype && \ | ||
| python manage.py loaddata 'fixtures/multivector_fixture.json' && \ | ||
| echo yes | python manage.py collectstatic && \ | ||
| pre-commit install && \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it added to the dependencies the developpers need to install in the README steps? If not can you add a it there?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch, I did forget to update the readme.
@paulapreuss - are you sure? When I checked out the branch the history did not build on top of |
…t have duplicate and or converging requirements
You're right, apparently I had not pulled the most recent changes. I'll rebase it again. |
de0c562 to
1c045f4
Compare
Bachibouzouk
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @4lm for initiating and @paulapreuss for updating it :)
This PR is based on unmerged PR #197.
Closes #215.
@Bachibouzouk, please review this PR and merge on approval. But first, PR #197 needs to be fixed as spoken by you and merged because this PR depends on it.
This PR uses
pre-commitfor installing and managing Git hooks.Initial installation:
pip install (app/)requirements.txtpre-commit installThe config can be found in
.pre-commit-config.yamlin the root folder.Hooks run before every commit.
Run changed files manually:
pre-commitRun all files manually (recommended after installing a new hook):
pre-commitPlease note: I configured that hooks shall fail fast. This can result in running the commit (or pre-commit) multiple times. If we realize that this behavior is different from what we want to see, we can change it in the config in the future.