f2e-spec Add git hooks

What does this PR do?

It adds some git hooks devided in necessary and optional.

The necessary scripts check if tabs instead spaces are used and there is no trailing whitespace.

The optional scripts autocorrect tabs and trailing whitespace.

Closes Issue(s)

Partly closes: https://github.com/performous/performous/issues/514

Motivation

The hooks help to commit correctly styled code.

Additional Notes

Actually there is only a powershell install script, which also work on linux with pwsh.

Tasks

[ ] Missing extensions: xsl, frag, vert, geom, yml, cmake, config, hpp, inc, ebuild

Apr 06 '22 21:04 twollgam

If this PR is merged I will update the style guide in the wiki how to install the git hooks and what they do.

Apr 06 '22 22:04 twollgam

Hmmm i'm having double feelings about the githooks. It requires the developer to install these into the repository as it doesn't get configured upon clone the repository. After install it blocks and/or modifies your commit if it doesn't agree with the style itself.

This is a good thing in general, however like i pointed it it requires an installation/configuration from the developer. However the actual issue is a 'server' problem. The server (which can also be described in other words: performous-core owners) want to enforce a specific styling. Therefore it is/can become a server problem which the server should resolve.

I'd suggest to wait with this PR until we have #715 merged. And then add a style-checker job. If the issue can be automaticly resolved a bot could create this style-fixing-commit with the approach you already made in these scripts. If the issue cannot be automaticly resolved, the developer should add another commit resolving the issues. This is indicated by a failing github action which need to be resolved eitherway.

To aid the developer to not make these mistakes in the first place, we still can suggest to install these commit hooks

@twollgam what do you think about that approach?

Apr 13 '22 10:04 Baklap4

@Baklap4 I agree what belongs the server side. Not to offer server-side hooks may be a decision for security reasons but it denies solutions which could reject commits. If we can make a server-side solution I would prefer this.

A solution that only checks and remarks false style is not what I expect. Those checks must run early. I do not want something where I have to do "extra style commits".

I use the hooks in this PR and they are not perfect yet but I like to be warned and I like the "auto correction" ;-)

BTW: once installed, the hooks update themself from repo.

Apr 13 '22 15:04 twollgam

Not to offer server-side hooks may be a decision for security reasons but it denies solutions which could reject commits.

That's correct it's for security reasons github doesn't want you to execute 'random' code on their servers ;)

A solution that only checks and remarks false style is not what I expect. Those checks must run early. I do not want something where I have to do "extra style commits".

Well in a way this is the same as when it rejects it locally. The gh-actions would fail, so the developer needs to do something to fix it. This can be done by either amending the relevant commits and force-pushing or by creating a last commit 'cleanup'. Which is in a way pretty much the same locally: it rejects your commit, you have to make changes and then commit the change

Like i said before the installing of hooks require the developer to do something, it'd be encouraged, but if not installed the ci/cd will tell you to clean up your "mess"

Apr 14 '22 08:04 Baklap4

Download the artifacts for this pull request:

This service is provided by nightly.link. These artifacts will expire in 90 days and will not be available for download after that time.

Apr 22 '22 14:04 github-actions[bot]

The scripts are nice, but I think I would have put them in a workflow preventing merge if the files your are committing contains illegal code

Apr 22 '22 14:04 yoda-jm

The scripts are nice, but I think I would have put them in a workflow preventing merge if the files your are committing contains illegal code

Best would be to have them on either side. So within the workflow and installed in your local repository.

@twollgam can you rebase this branch and try adding the checks to the workflow (ask for help if you need any) so we check it on both sides (server: GH-Actions and client: pre-commit hooks)? If the check is not succesful the action will result in a failure, and by making it a required action we prevent it from getting merged.

Apr 25 '22 09:04 Baklap4

Hmm after installing the hooks (with option 'all' defined) i saw it didn't work with other text based files due to the extension constraint. Please add the following to the list:

.md

.html

.css

.js

.xml

.json

.txt

.desktop

.sh

.ps1

.bash

other text based files

@Baklap4 what would you do expect to be checked for this files? No white space at end of lines - ok, but other formatting like tabs too? I split the checks into seperate scripts so that checks can be done for different types of files.

May 18 '22 16:05 twollgam

@Baklap4 what would you do expect to be checked for this files? No white space at end of lines - ok, but other formatting like tabs too? I split the checks into seperate scripts so that checks can be done for different types of files.

Yeah both can done and 4 spaces/1tab seems pretty much the norm out there anyway regardless of what kind of textfile there is

May 18 '22 17:05 Baklap4

stumbling upon this old pr again. Within the format project we use pre-commit. This project already defines a lot of standard commit hooks which we can integrate. Maybe that's nice to do aswell? https://github.com/UltraStar-Deluxe/format/blob/main/.pre-commit-config.yaml

Nov 26 '23 20:11 Baklap4

f2e-spec f2e-spec copied to clipboard

Add git hooks

What does this PR do?

Closes Issue(s)

Motivation

Additional Notes

Tasks

f2e-spec
f2e-spec copied to clipboard