pre-commit setup #238
pre-commit setup #238
Conversation
|
@bmispelon This is still WIP but please let me know if there is something to update / change / fix. |
|
This is looking very good so far, thanks for tackling it! 🎸 One thing I noted is that the template formatting doesn't seem to understand Trac's weird jinja configuration. See for example the changes proposed in |
.pre-commit-config.yaml
Outdated
| - id: end-of-file-fixer | ||
| exclude_types: [json, sql] | ||
| - id: file-contents-sorter | ||
| files: ^(requirements/\w*.txt)$ |
There was a problem hiding this comment.
In this project there's only a single requirements.txt file
| files: ^(requirements/\w*.txt)$ | |
| files: ^requirements\.txt$ |
There was a problem hiding this comment.
I updated the related commit, should be okay now.
There was a problem hiding this comment.
I guess I messed up the rebase, sorry. Double checked this time, looks okay.
There was a problem hiding this comment.
It seems this hook doesn't add value to this repo. The requirements.txt file has a special organization, which gets destroyed when package names are ordered alphabetically. I removed it.
ulgens
force-pushed
the
pre-commit-setup
branch
2 times, most recently
from
147388a to
23c9516
Compare
I agree with this. Since the files are Jinja templates for Trac, and the checker was looking for Django templates, I removed |
.pre-commit-config.yaml
Outdated
| @@ -0,0 +1,63 @@ | |||
| default_language_version: | |||
| python: python3 | |||
There was a problem hiding this comment.
While comparing this configuration file to the one from djangoproject.com I noticed you're using 2 spaces for indentation while the other file has 4.
Could you change this file to 4 as well, it will make it easier for me to review this PR, and will also make it easier to integrate changes between the two files in the future.
There was a problem hiding this comment.
I missed the .editorconfig file, sorry. Once it was in place, the indentation for yaml files became 4 spaces.
.pre-commit-config.yaml
Outdated
| # Trac needs psycopg2 | ||
| # https://github.com/adamchainz/django-upgrade?tab=readme-ov-file#databases | ||
| --skip, | ||
| "settings_database_postgresql", |
There was a problem hiding this comment.
I might be misunderstanding what the settings_database_postgresql fixer does, but I don't think it has to do with psycopg2 vs. psycopg3. It's just a naming alias change and you can use django.db.backends.postgresql with psycopg2.
There was a problem hiding this comment.
Addressed this under #243
.pre-commit-config.yaml
Outdated
| - id: end-of-file-fixer | ||
| exclude_types: [json, sql] | ||
| - id: file-contents-sorter | ||
| files: ^(requirements/\w*.txt)$ |
ulgens
force-pushed
the
pre-commit-setup
branch
2 times, most recently
from
738aa7c to
b7e0746
Compare
ulgens
force-pushed
the
pre-commit-setup
branch
2 times, most recently
from
885d5bf to
7504d6d
Compare
ulgens
force-pushed
the
pre-commit-setup
branch
2 times, most recently
from
ff90427 to
af783e9
Compare
There was a problem hiding this comment.
This is looking great now, we're almost there! 🏁
I have one last question about the prettier commit hook: does it makes sense to keep it in this project (see comment above exclude_types).
Once that's clarified, I think we're good to go.
Thanks again for working on this!
ulgens
force-pushed
the
pre-commit-setup
branch
from
af783e9 to
ab46da9
Compare
Resolves #237
Trying to import pre-commit config from https://github.com/django/djangoproject.com/blob/ede018f69a7ae5ecacb99978d067f53613d05353/.pre-commit-config.yaml
Differences:
Missing
requirements.txtfiles already have a different and special ordering.Changed