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

feat: add django-diagrams hook #9

Draft
wants to merge 5 commits into
base: master
Choose a base branch
from
Draft

Conversation

Gen-QR
Copy link

@Gen-QR Gen-QR commented Jan 3, 2024

This hook will add/update diagram files in the docs directory of a django app when there are migration files present in the commit. The aim is to give a model relationship reference for developers and product owners. This is not intended for end user consumption, it is just an internal reference.

@dave-v dave-v self-requested a review January 3, 2024 10:15
@Gen-QR Gen-QR marked this pull request as ready for review January 4, 2024 08:20
@Gen-QR Gen-QR marked this pull request as draft January 4, 2024 08:21
@Gen-QR Gen-QR marked this pull request as ready for review January 4, 2024 08:22
@@ -20,3 +20,9 @@
entry: (has_perm|@permission_required)\(["'][^\."']+["']\)
language: pygrep
types: [python]
# when calling this hook in a repo you must add args: [<path to local settings e.g. qrhub.settings.local>] in the repo .pre-commit-config.yaml file
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we use the DJANGO_SETTINGS_MODULE env var?

Copy link
Author

@Gen-QR Gen-QR Jan 9, 2024

Choose a reason for hiding this comment

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

It would be better if we could but I couldn't find a way to get it into the .pre-commit-hooks.yaml file

Copy link
Contributor

Choose a reason for hiding this comment

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

This needs to use the env var, otherwise it won't be able to use the correct settings in both local and CI contexts. Not sure what you mean by getting it into the YAML file, but first thing to try would be echo $DJANGO_SETTINGS_MODULE in the bash script to see if it's set.

Copy link
Author

Choose a reason for hiding this comment

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

I tried that but will just double check

generate_diagrams.sh Outdated Show resolved Hide resolved
generate_diagrams.sh Outdated Show resolved Hide resolved

declare -A unique_apps

MIGRATIONS=$@
Copy link
Contributor

Choose a reason for hiding this comment

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

Won't this be empty?

Copy link
Author

Choose a reason for hiding this comment

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

No it shouldn't. The hook passes the migration file names as args (at least testing locally on the hub it does. It should behave the same way here)

Copy link
Contributor

@dave-v dave-v Jan 8, 2024

Choose a reason for hiding this comment

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

I see, then presumably it is passing all changed files and you need to filter out those that are not migrations? Or does the pre-commit files parameter do that filtering for you?

Copy link
Author

Choose a reason for hiding this comment

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

The pre-commit files param does it

generate_diagrams.sh Outdated Show resolved Hide resolved
python -m django_diagram -a $APP -o ./docs/model_diagrams/$APP.md -s $SETTINGS_MODULE
done

git add ./docs/model_diagrams/
Copy link
Contributor

Choose a reason for hiding this comment

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

Should make use of the $directory variable here too, but I am wondering if you should not include this command, as I think hooks usually don't stage changes they've made? Apparently it's discouraged.

Suggested change
git add ./docs/model_diagrams/

Copy link
Author

Choose a reason for hiding this comment

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

generate_diagrams.sh Show resolved Hide resolved
@Gen-QR Gen-QR requested a review from dave-v January 9, 2024 09:30
@Gen-QR Gen-QR marked this pull request as draft January 11, 2024 08:51
@dave-v dave-v removed their request for review March 27, 2024 09:03
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.

None yet

3 participants