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

Add format workflow #66

Open
wants to merge 6 commits into
base: dev
Choose a base branch
from
Open

Add format workflow #66

wants to merge 6 commits into from

Conversation

WhySoBad
Copy link
Collaborator

This pull request adds a github workflow which checks for correct code formatting.

I created a separate workflow file for the format check which is then run before the build job. This way it can be reused should there ever be another workflow which needs to run after the format check step.

@WhySoBad WhySoBad changed the base branch from main to dev November 21, 2024 14:21
@WhySoBad WhySoBad self-assigned this Nov 21, 2024
@WhySoBad WhySoBad marked this pull request as ready for review November 21, 2024 14:22
@WhySoBad WhySoBad added the enhancement New feature or request label Nov 21, 2024
@levnikmyskin
Copy link
Owner

Thank you very much for taking the time for this. I was thinking: should we also use pre commit? I can take care of adding this over the weekend

@WhySoBad
Copy link
Collaborator Author

I don't fully understand what you mean by pre-commit. Do you mean to run the formatter in a git hook before committing or some kind of github workflow?

@levnikmyskin
Copy link
Owner

Yes, a git hook. We can use https://pre-commit.com/

@WhySoBad
Copy link
Collaborator Author

Yeah, sounds good. Do you mind adding it yourself? I don't know much about git hooks and probably won't find the time tomorrow to implement it

@levnikmyskin
Copy link
Owner

Yeah will do it myself! Not in the next days probably, as I'm a little busy with personal stuff, but will try to not forget about this!

@levnikmyskin
Copy link
Owner

hey @WhySoBad , finally added the pre-commit stuff. You can follow the instructions on https://pre-commit.com/ to install it. Also added a few other things. Let me know if this works for you!

@WhySoBad
Copy link
Collaborator Author

WhySoBad commented Dec 1, 2024

Hi,
I'm wondering why you chose to rely on an external program to install the pre-commit hook. We could simply create a Makefile target which installs a shell script into .git/hooks/pre-commit which then runs the formatter.

I think not having to rely on a 3rd party would be beneficial as it does not require every collaborator to first install another program just to install the script which runs the formatter. If we keep the current implementation I fear most people will simply not use it as the barrier for entry is too high.

Edit:
To keep all the features which are available with the current implementation we could simply version control the generated shell script and then install this using a Makefile target

@levnikmyskin
Copy link
Owner

I get your point, fair concern. There was no particular reason other than I already use this at work, and it was really easy to setup.
But I get it, and I'm also down to making it simpler, also considering this is not a professional project, with a rather simple codebase. I'll look into it, and come back to you

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants