docs: update commit section of engineering workflow

[mc-jones]

This PR updates the commit section of the engineer workflow description. It supports RFC #327 which proposed conventional commits as Artsy's commit message convention and aims to improve adoption across the engineering team.

Importantly, it also supports ongoing work to track the ratio of new to rework by parsing through commit messages on Artsy Repos. See relevant slack thread.

The list of accepted types is based on commitlint. See relevant slack thread.

Note: I'll share in #dev before merging, but wanted to be more limited in those who have already commented on this in the first review. But feel free to add other reviewers.

[mc-jones]

@Mitch-Henson

To confirm, this only applies to PRs right? I remember a conversation within a slack thread (that I can't find with a cursory search) where this seemed to be a hot topic. If we are squashing all commits when merging then I guess only the PR title is relevant

The measure looks at all commits on the main branch. So if you squash it only applies to the PR title. But if you do a pure merge then all commits will be counted. So if a straight merge is the preferred merge method, the submitter and reviewers/assignee should make sure each commit in the PR follows the convention.

Was the slack thread you're thinking of this one between @damassi and I talking about Force?

[Mitch-Henson]

@mc-jones that wasn't the slack thread I was thinking of. I was referring to one at least a few months old but is still in my mind. I think @jonallured was in it and there were very strong opinions for both points of view. I was impressed how polite the discussion was for how strong the opinions were.

The TL;DR of the slack thread I was mentioning was that as long as we don't dictate how each single commit needs to be structured (anecdotally most PRs as we use squash), then only the PR structure is relevant which I agree with. So totally happy for this change to go ahead as I believe that PR's should be standardised anyway

[jonallured]

I'm confused about this PR because it's labeled as a feat but reads like an RFC. I strongly disagree with this change and would encourage us to not merge it without due discussion!

I have no problem with this structure at the PR level but I believe this is too prescriptive at the commit level. Steve Hick wrote a nice blog post about this very thing here:

https://artsy.github.io/blog/2020/08/11/improve-pull-requests-by-including-valuable-context/

And I'll take the PR of mine that he called out as doing this well:

https://github.com/artsy/convection/pull/645

Here's the log of changes:

* 1d9afdf Offer params should override defaults
* b52c5d8 Extract locals around creating an offer
* d2b7831 Do some reorganization of spec file
* 4316914 Simplify specs when there is a PS
* 91eba9c Use the rspec change matcher when possible
* d793c44 Simplify error assertions
* a10a54e Only use reload when you have to
* 756311f Run prettier
* ec4fa9b Use consts for offer states
* 94a5d20 Use lazy load unless you can't
* a66e684 Move to using consts instead of strings
* 0875eaa Promote submission fabrication, leave state
* 1bd026c Use more generic name
* 605eb73 Push fabrication down
* 340e37d Extract local
* 7c81e45 Forgot one

If this work had followed the commit-level rules of this PR then it would look a lot different. I think we'd leave off the scope of each commit (right?) but I'd be asked to include the type? So maybe that looks like this:

* 1d9afdf feat: Offer params should override defaults
* b52c5d8 refactor: Extract locals around creating an offer
* d2b7831 refactor: Do some reorganization of spec file
* 4316914 refactor: Simplify specs when there is a PS
* 91eba9c refactor: Use the rspec change matcher when possible
* d793c44 refactor: Simplify error assertions
* a10a54e refactor: Only use reload when you have to
* 756311f chore: Run prettier
* ec4fa9b refactor: Use consts for offer states
* 94a5d20 refactor: Use lazy load unless you can't
* a66e684 refactor: Move to using consts instead of strings
* 0875eaa refactor: Promote submission fabrication, leave state
* 1bd026c refactor: Use more generic name
* 605eb73 refactor: Push fabrication down
* 340e37d refactor: Extract local
* 7c81e45 fix: Forgot one

I don't think this is better in terms of an engineer reading the history to discover how things have changed over time.

Does this format help with the analysis of our work? Does this tagging ensure the ratio comes out right? Maybe? But why not point our analysis at PRs rather than commits! I'm proud of the work I did there to create a history that my PR reviewers could read and digest and I believe it would be a shame if our docs directed folks away from this type of approach.

[admbtlr]

My understanding here is that we're only interested in commits that end up in main. So if you're squashing commits on merge, then it's only the PR title (or manually entered commit message) that matters, not the individual commits. If you're not squashing, then yes you will need to prepend all your individual commits, as we've discussed in the past.

This isn't any different to what we're already doing, is it? Or am I missing something @jonallured ?

[mc-jones]

@admbtlr - @jonallured and I chatted quickly last week on this and I'm going to amend this a bit. Specifically, I'm going to include a line to clarify that, in instances where developers feel it will result in a more precise commit history to not use a conventional commit message for a specific commit that will be merged into main, they may do so (this doesn't apply to PR titles).

The goal of this is to continue to encourage conventional commits as our standard, but allow for some flexibility to use a good, but not spec-adhering, commit where it may not be the most appropriate.

[mc-jones]

Also, following my own suggestions, I'm going to change the type from feat to docs.

[damassi]

The issue @jonallured is that this PR in your example was mostly likely not squashed (knowing how you work), meaning each of those commits was not measurable by our system. And lots of PRs are only one or two commits, so adding the appropriate tag is pretty trivial.

Secondly, it shouldn't be up to the reviewer to remember to squash the commit; that's asking too much of the process. So its either: dev makes nice orderly commit history in the style that they like -- and then either rebases to squash the commit before merge with an extended commit message, or instructs the reviewer to only "approve" the PR so that the dev can update github UI to squash and merge.

All of this complexity could be alleviated by tagging the commit with what it was. Sure, every one of those commits was a "refactor", but is this representative of a larger body of work? And additionally, if the history wasn't squashed, and one of those refactor commits introduced a bug (as refactor commits usually do) how is another person going to come along during a git bisect and know where to start? There's no indication that such and such commit in your example is a refactor, leading to wasted time.

[jonallured]

meaning each of those commits was not measurable by our system

Right @damassi which is why I'm saying we should measure at the PR level NOT the commit level. I think this is the direction you're headed, right @mc-jones?

The goal of this is to continue to encourage conventional commits as our standard, but allow for some flexibility to use a good, but not spec-adhering, commit where it may not be the most appropriate.

I support this goal and appreciate the flexibility! 🙌

[mc-jones]

I think this is the direction you're headed, right @mc-jones?

I'm happy with this path as I believe it is the best tradeoff to achieve the goal of allowing us to meaningfully evaluate the commits without restricting developers' ability to make judgments about when valuable context would be lost by forcing the convention.

I also want to emphasize that part of this effort is to use the data to better inform our practices. Once we share this, if we are finding that there are still a large number of unconventional commits, we can relook at this with actual data points.