OEP-0051 Conventional Commits

[nedbat]

This is an OEP for Conventional Commits. This establishes the guidelines, points at the third-party public spec, and
clarifies what we will and will not do.

Our goal now is to have an OEP that we can put into practice, then iterate.

Previous review and discussion is in another pull request: #182.

[kdmccormick]

I have a few suggestions, but I'm largely in favor of everything here.

[kdmccormick]

It's currently common at edX to put the ticket number at the beginning of the subject line; eg JIRA-1234 we made a change

In spirit of keeping commit subjects short, should we recommend that ticket numbers (if included) should go in the body?

[inventhouse]

Or at the end of the subject so they still show up in git log --oneline, but don't get in the way as much

[nedbat]

Including them at the end of the subject seems good.

[inventhouse]

Somewhat tangental to the conventional commit format, but it might be good to specify, or at least encourage, standardizing on writing commit messages in the past tense. (I find logs much more readable when the tense is consistent, and past tense is more intuitive to me when looking back at history)

[nedbat]

About tense, it seems more common to require present tense. The Angular commit guidelines say:

use the imperative, present tense: "change" not "changed" nor "changes"

[pomegranited]

Glad to see this proposal! It's a great start at making the code history more comprehensible.

[pomegranited]

What Scopes should we use to mark and then remove deprecated code?

This post recommends using feat!: deprecate feat x first to deprecate the feature, and fix!: removed feat x to actually remove it. The initial deprecation notice doesn't actually break anything, it's just an announcement of a future breaking change.. But I'd like deprecations to be highlighted like breaking changes, since they require action to be taken soon.

(Can't rightly use refactor!: removed deprecated feat x, because it's a contradiction for refactor to include breaking changes.)

[nedbat]

This is an interesting point. I think a lot of the existing conventional commit guidelines are aimed at automated versioning. For that purpose, deprecation shouldn't be "feat!", because you don't want to bump the major version when you deprecate a feature. You want to bump it when you remove the feature.

We've been focusing our discussion primarily on people reading the commits, and you are right, they need to sit up and notice when a deprecation happens. One option would be a new label "depr:", but I also think the word "deprecate" will stand out:

feat: deprecate course creation

I'm in favor of labeling and signifiers, but there will also still be words. If we write them well, people will read them.

[pdpinch]

As long as the word "deprecate" appears in the commit subject, when the code is deprecated and when the code is removed, I think that is clear enough.

[nasthagiri]

Thanks for this discussion.

Can someone clarify for me where the discussion lands on what the commit message will be when a feature is actually removed? Would it use the feat! prefix or the fix! prefix with the removing commit?

[nedbat]

The removing commit should use "feat!", since it is a breaking change (presumably) to a feature. "feat" should be used for any change in the feature set.

[pomegranited]

Would be great if the edx-transifex-bot and edx-requirements-bot could be updated to use this new convention too.

[nedbat]

Absolutely, we'll be updating them.

[samuelallan72]

Would it be possible to modify this to highlight the importance of explaining why the change was made? Explaining what is important, but only an overview is required. One can read the code and code comments to see more about the what and the how, but the why can only really be explained well in a commit message.

[nedbat]

That's a good point, I will add more here.

[nasthagiri]

Can we also add a reminder that the Open edX best practice is to document the why of non-trivial changes in ADRs?

From the OEP, ADRs answer the questions:

* What technical decisions were made for the component/feature in this repo/app/folder?
* Why does this component/feature do X?

A PR's relevant ADR(s) can be committed as separate docs commits with the PR itself - or in a previous PR.

[nedbat]

I've added an "exp" type for exploratory/experimental changes that are meant to be temporary.

[samuelallan72]

@nedbat

About tense, it seems more common to require present tense. The Angular commit guidelines say:

use the imperative, present tense: "change" not "changed" nor "changes"

Could this be included in this OEP? It will help with consistency (which is partly the goal here).

[inventhouse]

About tense, it seems more common to require present tense. The Angular commit guidelines say:

use the imperative, present tense: "change" not "changed" nor "changes"

But they don't seem to give a rationale for that choice; do you know why? Commits will be in the past a lot longer than they will be in the present; furthermore, the examples in this proposal are both in the past tense.

[nedbat]

Thanks, I've corrected the tense of the examples. I'm reluctant to dictate the grammar now. We will be starting with these guidelines, and iterating.

[inventhouse]

Thanks, I've corrected the tense of the examples.

No, they were right before 😝 But seriously, I’m interested to know why some prefer present tense for commit messages. (Although I admit to sometimes writing them "in the moment" myself, I am actively trying to be consistent with the past tense because they read so much better after the fact)

[samuelallan72]

@inventhouse

But seriously, I’m interested to know why some prefer present tense for commit messages.

Because it ends up forcing a style that is more consistent and readable. It also allows the message to be a grammatically correct sentence. See https://chris.beams.io/posts/git-commit/#imperative for a good reference.

[inventhouse]

@swalladge interesting read, but it doesn’t explain the reasoning either, just an unsupported assertion about “a properly formed Git commit subject line”; for one brief moment “if applied, this commit will...” makes sense, but for the rest of the life of the project, it doesn’t. Most of the time, commit messages need to explain “what changed?” - past tense, the commit was applied, maybe years ago.

Anyway, all this is outside the scope of this OEP; I’m sure we’ll discuss this more when there’s a proposal to standardize commit message tense 😄

[stvstnfrd]

but it doesn’t explain the reasoning either

@inventhouse I believe it does:

Git itself uses the imperative whenever it creates a commit on your behalf.
...
So when you write your commit messages in the imperative, you’re following Git’s own built-in conventions.

Now, as to why git follows this convention, it's because the Linux kernel does,
both git and linux having been created by Linus Torvalds (with git being created to track Linux kernel source).

But why does Linux do this? They explain it as [1]:

Describe your changes in imperative mood, e.g. “make xyzzy do frotz” instead of “[This patch] makes xyzzy do frotz” or “[I] changed xyzzy to do frotz”, as if you are giving orders to the codebase to change its behaviour.

By using the imperative in our own commits, it maintains consistency and is easier to read, since we're already going to be "stuck" with messages in that tense.

While I won't go as far as saying this needs to be hard requirement at this time, I would support adopting it [2][3][4].
More so, I am opposed to enforcing the past tense.

Disclaimer: I've been recommending that article of best practices [5] for >5 years now, since including it in my original (Stanford) OpenEdX onboarding course. I write my messages in the imperative.

• [1] https://www.kernel.org/doc/html/v4.14/process/submitting-patches.html#describe-your-changes
• [2] Consistency is king; the less context switching, the less cognitive load.
• [3] It reads as a complete sentence.
• [4] It typically leads to shorter messages (in English, anyway) 😜
• [5] https://chris.beams.io/posts/git-commit/

[stvstnfrd]

I'm not a fan of this; it's opaque to me.

• That the descrption provides two possible _exp_lanations for the _exp_ression 😉 (experimental or exploratory) suggests to me that it's not as obvious as it could be.

I'd prefer temp or tmp, as these terms have broader adoption across the software landscape, and even beyond the technical domain; this would be clear to less technical readers too.

[nedbat]

I see your point. "temp" was my first choice. I will switch it.

[stvstnfrd]

Ideally, we would explicitly call-out the areas where we have diverged from the spec(s).

After reading both, it's not obvious to me (or someone coming into this project who already has experience w/ the original spec and/or Angular), what the immediate differences are.

[nedbat]

I will take a stab at summarizing the differences.

[inventhouse]

but it doesn’t explain the reasoning either

@inventhouse I believe it does:

Git itself uses the imperative whenever it creates a commit on your behalf.
...
So when you write your commit messages in the imperative, you’re following Git’s own built-in conventions.

As it appeared in the article, that still didn't explain the reasoning, however it does highlight an important gap in this OEP:

@nedbat Since git does not follow these conventions at all, we ought call out that git-generated commits should be avoided as much as possible, the most common being merge, and to strongly prefer fast-forward or squash merges to preserve all this fancy formatting.

@stvstnfrd You went on to explain the reasoning much better than that article did, thank you. I disagree and if / when we decide to codify the tense (and I think we should), I'll go into more detail; for now it has been deemed out-of-scope for this proposal.

[nedbat]

I've been thinking about the tense question (that is, the question about tenses, not that the debate here is getting heated! :) ), and I think I prefer past tense also, as evidenced by my initial examples. Release notes will be a big reason for writing conventional commits, and they are typically written in the past tense. I am not a fan of auto-generated release notes, but the smaller we can make the gap between commit messages and release notes, the better.

[inventhouse]

@nedbat excellent point; happy to go further into my reasoning if there’s interest, I’m just trying not to hijack this OEP too badly 😄

[pdpinch]

It seems like many folks have already adopted docs: for the annotation doc-a-thon. Should we mention that case explicitly, or is this a one-time effort?

Suggested change

	* **docs**: documentation-only changes. Documentation changes for other types of work should be included in those commits. This includes more than the formal docs for a repo, it also covers READMEs, ADRs, docstrings, and so on.
	* **docs**: documentation-only changes. Documentation changes for other types of work should be included in those commits. This includes more than the formal docs for a repo, it also covers READMEs, ADRs, docstrings, annotations and so on.

[nedbat]

Absolutely annotations count. How about, "it also covers any change that updates words describing the work, for example READMEs, ADRs, docstrings, annotations, comments, and so on."

[pdpinch]

Please only include Jira issue numbers if the Jira issue is public.

[inventhouse]

I think we should always include jira numbers if they exist, even if not public, but:
a) the commit message still needs to stand on its own; the ticket reference should be purely supplemental even if it is public
b) we should endeavor to keep as many of our jira projects and tickets open to the community as possible

[nasthagiri]

@nedbat Do our automated tools allow for Jira numbers to be in the description instead of the subject-line? If so, the description seems a better location for "more information".

Given the character-length limitations of the subject-line (even from a readability POV), I'd prefer to use that real-estate for more significant information. ARCHBOM-xxx, VAN-yyy, DISCO-zzz don't provide meaningful info on the change itself.

Links to tickets are good for detective work as a backup solution - when the code change (PR, ADRs, code docs, etc) neglected to provide self-containing information. Directionally, I see us benefiting from removing our reliance on scrum-project-task-management-tickets as the sole source of information.

[inventhouse]

I definitely want the ticket numbers to be visible with git log —oneline, especially with long log bodies; I think they should be put at the end of the subject but not “counted against” the length.

[inventhouse]

@nedbat maybe we need to clarify that the primary ticket/issue number should (rather than "can") be in the subject so they can be seen with log —oneline

[inventhouse]

@nasthagiri Wow, there's a lot to unpack; here are a few quick thoughts...

Tickets link to the past: As long as we use tickets of some kind to track work, they will continue to be significant historical artifacts for the changes that result; they will tend to have details and clues that were rightly (at the time) summarized away or simply missed from the commit log. We must not lose them.

Tickets link to the future: Once a commit is merged to the main branch it can never be modified, but a referenced ticket can be updated with information or links after-the-fact; we have no other way to do this, and we must not lose that.

The connection from a commit to the significant ticket or tickets can be direct and reliable if the commit includes the reference, or (in my experience) a tenuous, unreliable, mini-sleuth (or not-so-mini!)

Even if the ticket is not currently public, it may be made public in the future, but the reference can't be added post-facto. Even if it is not made public, the reference itself leaves a clue that can be inquired about; the easier it is to see if there might be more information, the easier it is for internal engineers to help the community.

I sincerely believe the importance of including ticket references, no matter how good the commit message might be, cannot be overstated.¹

Separate but related, most of the time I find git log much too verbose for showing more than a commit or two; the bigger picture is completely swamped by the details and I have no sense of "where I am" after a couple screens; thus I use --oneline a great deal. Ticket references are the best, shortest, way I've found to "pop open" more information right from the space-constrained subject line. Although the full commit message may (hopefully) be better curated, it takes more steps to open and tends to lose my place in the list of logs; if the ticket looks promising, I'll certainly read the log too. I don't want to lose this option, and I think putting the reference at the end of the subject is a good compromise.

Footnotes

1. Seriously, I think every commit should reference a ticket, though I'm not (presently) proposing that requirement. ↩

[nedbat]

(BTW: our OSPR tooling looks in PR titles, not commit messages)

[nedbat]

I'm going to side with the "links in the body" side here. The subject is best used for words that explain. Supporting information, including references to Jira or GitHub issues should be in the body. It's harder to get to, but we're asking people to write longer commit messages. We need to get better at using those messages.

[inventhouse]

Oh well; I made my case and I’ve started to get some ideas for tools I can write to help work without the easy ticket references; hopefully they can help others as well.

[inventhouse]

(But definitely include those references in the commit message!)

[pdpinch]

This is really important, because I see a lot of feature changes arrive in edx-platform under the commit message "Updating Python Requirements"

But I fear it's going to be hard for the committer of a dependency update to manage.

Consider this commit edx/edx-platform@7c9513d -- I believe the committer was intending to update astroid but they also picked up an update to the edx-sga xBlock (a bug fix in this case).

Should the committer be responsible for tracking down the nature of each of the dependency updates? Or do they have a way to scope their commit so it doesn't go beyond the chore they intended?

[nasthagiri]

I'd imagine that the "chore" of updating python dependencies could pull in newer versions of multiple repos. It's likely that one or more of the sub-repos could have feat, revert, docs, etc changes in them. I don't believe there's information-value in having dependency-upgrade PRs muddy the semantics of the change by (for example) marking with the "maximum" of the commit-types. As a consumer, you lose information that these changes were brought in through upgrades.

I wonder if better tooling to visualize/summarize the commits taken from the sub-repos could help here insetad. CCing @jmbowman and @awais786 since their team is likely impacted and can provide ideas and input.

[nedbat]

This I think is going to be a real challenge. The entire motivation for conventional commits is to make it possible to easily understand the scope of a change from the commit message. Commits like "chore: automated requirements update" will be a huge gap where significant changes can appear without indication. I'm not sure what to do about this.

[nasthagiri]

I defer to @jmbowman on this. I'm good with what is feasible for Arbi-BOM's continual-upgrade efforts.

[OmarIthawi]

I like the change. No further comments. Thank you!

[nasthagiri]

Looks great overall!
I add a few comments with the intention of strengthening the proposal.

[nasthagiri]

"If a commit does, first consider breaking the commit into multiple commits per their semantical changes. If not possible (are you sure?), use the most important type in the commit message...."

[nedbat]

I don't know how important it is to keep every commit pure by type. If I make a commit that updates a large comment, and nothing else, that's a "docs" change. But that doesn't mean that all changes to comments should be in a separated "docs" commit. How bad is it if a feature commit also includes a fix?

(This is why I want to eventually get to commits including updates to changelogs: it gives us the space to fully describe what is happening.)

[nedbat]

If I implement a feature and write docs for that feature, should they be in two separate commits? I don't understand what value it would add to split them up like that?

[nasthagiri]

That's totally fair - especially regarding docs.

In my mind, I was thinking about keeping the following types of commits separate (as examples):

• refactor commits separate from feat commits - so it's easier for consumers to understand what functionality actually changed.
• 'style' commits separate - so consumers can easily validate those changes in a separate commit and spend more time on other substantial changes.
• 'temp' commits separate - allows easier removal of those changes in the future.

This suggestion is not a blocker for this OEP. But a suggestion of this sort can help authors improve the presentation of their code changes and can help consumers better understand the changes.

[nasthagiri]

@nedbat Do our automated tools allow for Jira numbers to be in the description instead of the subject-line? If so, the description seems a better location for "more information".

Given the character-length limitations of the subject-line (even from a readability POV), I'd prefer to use that real-estate for more significant information. ARCHBOM-xxx, VAN-yyy, DISCO-zzz don't provide meaningful info on the change itself.

Links to tickets are good for detective work as a backup solution - when the code change (PR, ADRs, code docs, etc) neglected to provide self-containing information. Directionally, I see us benefiting from removing our reliance on scrum-project-task-management-tickets as the sole source of information.

[nasthagiri]

Can we also add a reminder that the Open edX best practice is to document the why of non-trivial changes in ADRs?

From the OEP, ADRs answer the questions:

* What technical decisions were made for the component/feature in this repo/app/folder?
* Why does this component/feature do X?

A PR's relevant ADR(s) can be committed as separate docs commits with the PR itself - or in a previous PR.

[nasthagiri]

Thanks for this discussion.

Can someone clarify for me where the discussion lands on what the commit message will be when a feature is actually removed? Would it use the feat! prefix or the fix! prefix with the removing commit?

[nasthagiri]

I'd imagine that the "chore" of updating python dependencies could pull in newer versions of multiple repos. It's likely that one or more of the sub-repos could have feat, revert, docs, etc changes in them. I don't believe there's information-value in having dependency-upgrade PRs muddy the semantics of the change by (for example) marking with the "maximum" of the commit-types. As a consumer, you lose information that these changes were brought in through upgrades.

I wonder if better tooling to visualize/summarize the commits taken from the sub-repos could help here insetad. CCing @jmbowman and @awais786 since their team is likely impacted and can provide ideas and input.