-
Notifications
You must be signed in to change notification settings - Fork 181
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
[#771] add pre commit hook for linting and formatting #875
Conversation
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 a lot for your contribution @smlabt 🙏🏼
I'really like it.
Would you mind to create a very basic developing.md
or something else where you quickly explain how to use and setup the pre-commit hook?
A general point for discussion: do we want to add the linting also as a check in our CI pipeline
@@ -0,0 +1,4 @@ | |||
#!/usr/bin/env sh |
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.
Please add an apache header here.
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.
Hi @bossenti,
thanks for your feedback :) I will add the missing apache header and also some documentation for the initial setup.
I'm also already working on a CI integration, but I think we should open a second PR for this. This second PR will be very large, because we have to "prettify" all existing files, in order to pass the CI pipeline. And I think this is not closely related to the pre commit hook. What's your opionion on this?
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.
great to here, +1 for having a dedicated PR for the reformatting
With regard to our Java codebase we apply the code style check module by module in #820.
Do you think splitting up would also make sense for the UI somehow although we don't have the same module structure 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.
Hi, thanks a lot for the PR @smlabt.
I'm in favor of activating it module by module (if that is possible), since I'm not sure if a "full" automated reformatting might have some unexpected side effects.
I guess it makes sense to first get a bit more comfortable with the styling guidelines. What do you think?
Closes #771