Moving pre-commit hooks to monorepo

[jhiner]

Looking at moving dapp and contracts to monorepo, I see that the dapp project enforces a pre-commit hook using husky, applying linting rules to the code but also the commit message.

It doesn't make sense to keep that as a dapp-exclusive thing. We need the same commit message linting for the entire monorepo, not on a per-project basis, obviously.

So as a group, we need to decide, do we remove commit message linting from the monorepo for now, or use the rules from the dapp project? Personally, I think we should ditch commit message linting for now. We can always introduce it later if we think it'll add value.

What do you think? Feedback welcome... 👍 for removing commit message linting or 👎 add it to the monorepo.

[princetonbishop]

I actually think even moving to a monorepo behooves us to use commit message linting.
Imagine if we have a system with fix(frontend): commit message and feat(backend): commit message, one can easily identify. it brings some form of clarity in the monorepo. With that said, I'm for keeping commit message linting

[nichosystem]

yea that seems persuasive to me

[jhiner]

Unless we change the linter to enforce ..(project): structure, you are still relying on humans to obey the convention... which we can do w/o a linter also

Yes I share this opinion. The linter can only do so much, and we will still end up relying on humans doing code reviews to help enforce it.

[scooprinder]

Definining something like https://github.com/greenkeeperio/monorepo-definitions/blob/master/CONTRIBUTING.md#commit-message-guidelines would be useful IMO.

[zelkibuilds]

Unless we change the linter to enforce ..(project): structure, you are still relying on humans to obey the convention... which we can do w/o a linter also

If we decide to keep the linter I'd say this would be the way to go.

[jebsley3]

I don't see the benefit of commit message linting to be honest
Commit messages should be descriptive enough that you don't need an enforced structure to them to understand what's going on, and if someone is writing bad commit messages putting fix: or wip: isn't going to improve it much
I vote we have a doc with guidelines and suggestions instead

[zelkibuilds]

You can't prevent someone from writing bad commit messages either way. But between having a bad commit message that starts with feat/fix/refactor etc and just having a bad commit message one of these at least attempts to communicate the intention of the commit regardless of how bad the rest of the message is.

It also allows you to have automated version bumping if we'd want since one of the ways lerna supports this is through conventional commits messages and it only considers the subject for it (so fix/feat etc).

It does not fix the issue of badly named commits but between refactor: bad message and bad message if I'm looking at the git history in search of a specific point in time one if more informative than the other.

If the contributor is so bad at naming commits that they botch both the subject and the message itself (so using a chore when it's a feat for example) then I'd say that's a problem that has to be solved with the contributor and not through our conventions/tooling. The best we can do is guide people into doing it right.

[zelkibuilds]

Agree with the need for a CONTRIBUTING.md though, 100%

[zelkibuilds]

Don't have strong feelings about linting or not. I think linting helps with the commit messages having a predictable structure when someone is reviewing it and rails people into respecting the project's conventions which I don't see as a bad thing.

On the other hand what's really important imo is that whoever is reviewing makes sure whatever commits are being merged have messages that are relevant outside of the context of the PR and that they are easy to parse.

Keeping the current dapp convention but adding a context like princeton metioned makes the most sense to me. So something like fix(dapp): something where something is descriptive enough that when being read outside of the context of a PR is enough for whoever is reading the git history to understand what was changed.

If we end up following a convention for naming our commits I don't see how linting would be detrimental. I also don't see a reason not to have a project convention for commit messages as it helps keeping things consistent with rotating contributors/reviewers.

tl;dr linting or not I believe having a convention for naming the commits is something we'd want to make the git history easy to parse. Reviewer has to ensure commit messages are relevant outside of the context of a PR, no tool solves this. Assuming we decide in favour of having a commit message convention I don't see how linting would be detrimental as it just enforces a convention you want to have.

[nichosystem]

one downside to commit linting I've encountered is that I can't use any git GUIs to make commits. sometimes I want to use VSCode or Github Desktop, so it's a minor nuissance in that regard.

I also recall when I was first contributing running into a couple issues with the commit linting on my setup (Windows+WSL). Onboarding new devs could be a bit more seamless if we dropped commit linting from our stack

[zelkibuilds]

Ahh ok didn't know about that; that's annoying. Why does it happen it happen though?

[nichosystem]

haha I assumed that was the case for everybody, I guess I didn't try too hard to fix it. the error I get is Unexpected identifier husky - pre-commit hook exited with code 1 (error)

If git GUIs still work for you then maybe this is fixable, which would negate my first point but give more emphasis to my second point (onboarding new dev friction)

[zelkibuilds]

I don't use git GUIs so I've never run into this. No idea what that error is, would have to investigate. But if if breaks these kind of workflows I'm less keen on keeping it tbh. Was not aware of this issue.

I think keeping the staged files linting is something we want so we prevent diffs that only change indentation for example. If this issue is only regarding the commit message linting I'm in favor for dropping it but keeping the staged file linting + formatting.