search

giellalt / giella-core

Build tools and build support files as well as developer support tools for the GiellaLT repositories.
https://giellalt.uit.no
GNU General Public License v3.0
7 stars 2 forks source link

Use pre-commit for linting and code consistency? #43

Open snomos opened 8 months ago

snomos commented 8 months ago

https://pre-commit.com

Works locally as well as part of CI.

WDYT @albbas @flammie @Phaqui ?

Phaqui commented 8 months ago

Sure. Just a few observations: To avoid creating more noise than it clears, everyone who commits to the repo should be on board, and use pre-commit, with the same hooks (defined in .pre-commit-config.yaml). The rules should also be on the softer side.

Scenario: Pretext: Person A does not use pre-commit. Person B does.

Person A commits non-formatted code. Person B pulls, and makes some changes. When B commits, A's unformatted changes will be included in B's commit, because B will format all code (unless B goes to some lengths to explicitly prevent it). This can look noisy.

It can cause more hassle than it alleviates if the pre-commit rules are too strict. Sometimes, the linter (or even code formatter) may not agree with some code - even though you specifically need it that way. It can be a good idea to at least start it off on the less restrictive side. Maybe start with only code formatting, and no linting altogether?

flammie commented 8 months ago

Yeah my past experience from trying to get project-wide pre-commit hooks is that they usually end up like described above, if it blocks commit (push) it can leave people who didn't break the code unable to work or annoyed. If it can be efficiently enforced which it looks like might be more possible nowadays it's a very good idea. I've used for myself in the past effectively.