Skip to content

Conversation

@petesfrench
Copy link
Contributor

@petesfrench petesfrench commented Feb 6, 2025

Done

  • Add .pre-commit-config.yaml
  • Create bash script to run djlint from package.json

QA

  • Install pre-commit
  • Check out this branch locally
  • Try to commit with an error in each of the cases found in .pre-commit-config.yaml (JS, CSS, HTML, PYTHON)

Issue / Card

Fixes https://warthogs.atlassian.net/browse/WD-19061

Screenshots

[If relevant, please include a screenshot.]

Help

QA steps - Commit guidelines

@webteam-app
Copy link

@samhotep
Copy link
Member

samhotep commented Mar 5, 2025

@petesfrench so far looks good (still reviewing), I just have an observation

  • If you tend to make many small commits, pre-commit can be brutal especially for unformatted pages
  • What do you think about pre-push hooks? We could run the pre-commit checks as a pre-push script

@petesfrench petesfrench marked this pull request as ready for review March 12, 2025 08:20
@petesfrench petesfrench requested a review from samhotep March 12, 2025 08:20
@samhotep
Copy link
Member

samhotep commented Mar 12, 2025

@petesfrench the script doesn't detect errors in the new commits, just in the staged changes. Also, we should add a note to the readme about copying the file to .git/hooks/pre-push

@petesfrench petesfrench changed the title Setup pre-commit config Setup pre-push config Mar 18, 2025
@petesfrench
Copy link
Contributor Author

@samhotep Good suggestions! I have made the changes to the script you suggested

HACKING.md Outdated
## Pre-push hook
We has a pre-push hook that will run anytime you try to push changes. To get it to work you need to copy over the pre-push script to `.git/hooks`. This can be done with the following command:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We has indeed xD

error_flag=1
fi

if ! npx flake8 "${scss_files[@]}" && black --check --line-length 79 "${scss_files[@]}"; then
Copy link
Member

@samhotep samhotep Mar 20, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be sth like python_files, right?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we support python files btw?

Copy link
Member

@samhotep samhotep left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@pete great work! Works very well for html, js and scss files.

Just a qn before I approve, do we want to support python linting in this PR as well?

@petesfrench
Copy link
Contributor Author

@samhotep It seems I managed to push some incomplete work, we should definitely be checking .py files. Could you take another look please

@pete
Copy link

pete commented Apr 11, 2025

@pete great work

I'm afraid I can't take credit.

@britneywwc
Copy link
Contributor

@petesfrench @samhotep Thanks for this piece of work! Are there any updates on this PR?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants