docs: OEP-0051, Conventional Commits #182

nedbat · 2021-02-01T20:24:43Z

A provisional OEP for conventional commits.

Original description:

Mostly, this just establishes the guideline, points at the spec, and
clarifies what we will and will not do.

timmc-edx · 2021-02-01T21:28:41Z

oeps/conventional-commits.rst

+* **build**: anything to do with building or releasing the repo: Makefile, tox.ini, Travis, Jenkins, GitHub actions, and so on.
+* **chore**: a repetitive task such as updating requirements or translations.
+* **docs**: documentation-only changes. Documentation changes for other types of work can be included in those commits.
+* **feat**: a new feature, or a change to an existing feature. Anything that changes the set of features.


One thing that's not clear in the CC spec is how to label generic code "improvements" that aren't exactly bug fixes, and don't introduce new or deprecate existing features, but just... improve them. Stability, security, usability, etc. Performance improvement is already called out as a separate type, but that still leaves this intermediate space ambiguous. (I've already run into this when upgrading a cryptographic signer to a newer, more trusted algorithm.) Sometimes these are user visible, sometimes not.

Would we err on the side of feat for these improvements? Or consider any improvement to be a fix? Or should there be a different type?

@timmc-edx, from the point of view of someone looking to use commit messages as raw material for release notes, I would very much prefer seeing such improvements under feat.

I think this is an interesting point. User-visible changes to features feel different than improved stability or better crypto. I'm not sure where I would put them. I think I would tend toward "fix", since they are making an improvement. The reason you made the change was because the existing features will work better than they did before.

Maybe the way to explain this is: "feat" is for changes that would get mentioned in user docs. It's about what the software does. "fix" is for changes to improve how the software does it.

Or did that only make it worse?

Anecdote: I've made a bajillion commits recently that amount to changing student.models to common.djangoapps.student.models, and have been wondering which commit type I shoud be using. The only visible change is that a deprecation warning is removed. It's not necessarily a bugfix, but it feels very generous to call that a feat.

As a future evolution, I'm thinking we would have folks include what area/app of the platform it affects (since that's useful to know at a glance). For example "grades", "courseware", "certificates", etc. So that would also add another layer of specificity.

Under the new proposal my example of a trivial refactoring (student.models -> common.djangoapps.student.models) becomes a change of type api:, which doesn't feel right.

I am leaning towards the original proposal, in which:

refactor doesn't change behavior

fix fixing an existing behavior (ux, HTTP API, or otherwise)

feat adds new behavior (ux, HTTP API, or otherwise)

Since the frontend replatforming, our direction is that edx-platform is getting fewer and fewer ux type changes, with the hope that the edx-platform UIs will eventually go away. Given that, I believe that the feats of edx-platform (or any of our microservices) are naturally going to be HTTP API changes; splitting out ux and api doesn't seem necessary.

Catching up on the whole thread here -

It seems problematic to me to deviate significantly from the established types that many projects already use, and which are supported out of the box by tools like semantic-release. And for what it's worth, it would also be different than what we're already doing in all our frontend repositories.

There's a lot of flexibility in the original proposal, which is based on prior art from other projects/tools.

I'd suggest that adding color to the types is what scope is for, and any given commit WILL have its commit message associated with it still to add even more context. If we want more flexibility, adding scopes rather than changing the types would go a long way.

If something doesn't fit exactly into fix vs. feat vs. non-version-impacting-types, a prefix can be chosen by the effect an engineer thinks it should have on the software version. When viewed in the release notes, a mildly mis-typed commit with a good message can presumably be forgiven and/or understood anyway with little impact.

Point being, between the existing types, adding a scope, and the message itself, plus any other identifiers we want to decorate our messages with, it seems like we have plenty of nuance to work with without inventing our own standard types.

The tweaks I suggested were based on the the assumption that it is useful to be able to tell programmatically:

If a change only impacts developers only or end users as well.

If it a large enough change to be worth highlighting in the release notes. Given the high volume of changes in edX a list of all the fix and feature commits is still going to be very long for anyone to process.

So the first question is whether it is useful to be able to programmatically differentiate in one or both of the above? If it is not then yeah we don't need to add anything beyond the base spec and only need to decide on the conventions for the ambiguous cases that were discussed earlier in the thread.

If however it would be useful to programmatically differentiate along either of the two axis, are there any other proposals? Though it does not look like we can use scopes since the spec says:

A scope MAY be provided after a type. A scope MUST consist of a noun describing a section of the codebase surrounded by parenthesis, e.g., fix(parser):

I would agree with @davidjoy and @nedbat about sticking closely to the spec. I think especially at the beginning since we haven't seen it in action yet. I think what @nasthagiri summarized is good enough for now and we can iterate if we find a lot of friction with this initial approach.

* performance improvement -> `perf` * security fix -> `fix` * usability -> `feat` * other types of code changes -> `refactor`

Mostly, this just establishes the guideline, points at the spec, and clarifies what we will and will not do.

nasthagiri

Looks great! Looking forward to using this in edx-platform with wider adoption in our other repos.

nasthagiri · 2021-02-01T23:20:42Z

oeps/conventional-commits.rst

+Tooling
+*******
+
+One of the advantages of formalized commit messages is using them as input to tooling and conformance checkers.  However, we are not proposing the use of these tools at the moment.


Arbi-BOM has plans in this quarter to add a linter to enforce conventional commits. cc: @jmbowman

Seems like we should coordinate on that :)

nasthagiri · 2021-02-08T23:30:26Z

oeps/conventional-commits.rst

+Type
+====
+
+We use these commit types:


What do we think about adding an api commit type? Are APIs that are built for Extensions designated as feat or do we introduce a new category for this?

"api" is the kind of thing that would be part of the scope if we were using them. Perhaps scopes is the solution to the "user-visible" aspect also?

symbolist · 2021-02-11T13:50:42Z

oeps/conventional-commits.rst

+
+    <body>
+
+    <footer>


Are body and footer required or optional?

nasthagiri · 2021-02-12T02:53:38Z

oeps/conventional-commits.rst

@@ -0,0 +1,77 @@
+####################


As a rollout process question:

Can we trial this proposed set of commit-types for a period of time and get input from devs (somehow) on what's working and what's not?

That is, can we provide a place for devs to provide ongoing feedback on what they find?

"None of these types fit for the type of change in this commit."

"Multiple types correspond to this commit and I don't see the value in separating them into multiple commits."

This feedback will then give us input on how, if at all, we need to ammend.

[suggestion] Would it be useful to have a google form that was open for input regarding this spec at all times? We could then collect issues over time and update the spec as needed. This would build a cheap feedback loop and if we can link it from the OEP, it would hopefully be pretty easy to find.

feanil

I have some suggestions for improvements in clarity but I think this spec is good enough to land as is. The only major change I would want to see is more clarity on how to provide feedback for future improvements or sources of friction. Once that's clear, I think this is good from my perspective and we should re-visit it in 6-months with feedback in hand to improve it further.

feanil · 2021-02-24T21:16:48Z

oeps/conventional-commits.rst

+* **feat**: a new feature, or a change to an existing feature. Anything that changes the set of features.
+* **fix**: bug fixes.
+* **perf**: performance improvement.
+* **refactor**: code reorganization that doesn't change behavior.


Suggested change

* **refactor**: code reorganization that doesn't change behavior.

* **refactor**: code reorganization that doesn't change behavior from the code consumer perspective.

feanil · 2021-02-24T21:18:33Z

oeps/conventional-commits.rst

+* **chore**: a repetitive task such as updating requirements or translations.
+* **docs**: documentation-only changes. Documentation changes for other types of work can be included in those commits.
+* **feat**: a new feature, or a change to an existing feature. Anything that changes the set of features.
+* **fix**: bug fixes.


Suggested change

* **fix**: bug fixes.

* **fix**: bug fixes including changes that remove or mitigate security vulnerabilities.

feanil · 2021-02-24T21:25:35Z

oeps/conventional-commits.rst

+Footer
+======
+
+We are not proposing any formalized footers in the commit message.


Suggested change

We are not proposing any formalized footers in the commit message.

We are not proposing any formalized footers in the commit message. Though the use of a ``BREAKING CHANGE:`` footer is encouraged as a way of providing more details about breaking changes.

Perhaps this just muddies the waters a bit? It doesn't need to be in there but I think given that this footer is well codified in the spec reminding people about it here would be useful.

Does the exclamation point syntax (feat!:) cover this case well enough?

In my use, I've used the message body to talk about the change as a whole and the footer to then talk specifically about the breakage and impact. Not needed, but given that this footer exists mentioning it here reduces a hop to know about it.

This is not a must have and doesn't need to block merging of this provisional OEP

feanil · 2021-02-24T21:28:57Z

oeps/conventional-commits.rst

+* **build**: anything to do with building or releasing the repo: Makefile, tox.ini, Travis, Jenkins, GitHub actions, and so on.
+* **chore**: a repetitive task such as updating requirements or translations.
+* **docs**: documentation-only changes. Documentation changes for other types of work can be included in those commits.
+* **feat**: a new feature, or a change to an existing feature. Anything that changes the set of features.


I would agree with @davidjoy and @nedbat about sticking closely to the spec. I think especially at the beginning since we haven't seen it in action yet. I think what @nasthagiri summarized is good enough for now and we can iterate if we find a lot of friction with this initial approach.

* performance improvement -> `perf` * security fix -> `fix` * usability -> `feat` * other types of code changes -> `refactor`

feanil · 2021-02-24T21:30:54Z

oeps/conventional-commits.rst

@@ -0,0 +1,77 @@
+####################


[suggestion] Would it be useful to have a google form that was open for input regarding this spec at all times? We could then collect issues over time and update the spec as needed. This would build a cheap feedback loop and if we can link it from the OEP, it would hopefully be pretty easy to find.

nedbat · 2021-02-25T21:26:15Z

About a feedback form: I don't know if people will remember where it is, and use it. Would it be enough to tell people to raise questions in the #dev Slack channel?

nedbat · 2021-02-25T21:32:23Z

All: I've updated the document and given it the OEP-0051 number. I'd like to merge it as a Provisional OEP, and start putting it into practice.

feanil

nedbat · 2021-02-26T18:22:37Z

This is re-opened in #189.

nedbat · 2021-02-26T18:28:05Z

We are looking for an arbiter to get this merged.

timmc-edx reviewed Feb 1, 2021

View reviewed changes

docs: first draft of conventional commits

e2054a7

Mostly, this just establishes the guideline, points at the spec, and clarifies what we will and will not do.

nedbat force-pushed the nedbat/conventional-commits branch from f6c9bca to e2054a7 Compare February 1, 2021 22:04

nasthagiri approved these changes Feb 1, 2021

View reviewed changes

nasthagiri reviewed Feb 8, 2021

View reviewed changes

symbolist reviewed Feb 11, 2021

View reviewed changes

oeps/conventional-commits.rst Outdated

<body>

<footer>

Copy link

symbolist Feb 11, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are body and footer required or optional?

nasthagiri reviewed Feb 12, 2021

View reviewed changes

docs: convert to oep-0051

9aae04b

feanil reviewed Feb 24, 2021

View reviewed changes

docs: oep-0051: include more commentary

af72356

nedbat changed the title ~~docs: first draft of conventional commits~~ docs: OEP-0051, Conventional Commits Feb 25, 2021

feanil approved these changes Feb 26, 2021

View reviewed changes

nedbat merged commit 2d97b03 into master Feb 26, 2021

nedbat deleted the nedbat/conventional-commits branch February 26, 2021 17:55

nedbat restored the nedbat/conventional-commits branch February 26, 2021 18:15

nedbat mentioned this pull request Feb 26, 2021

OEP-0051 Conventional Commits #189

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

docs: OEP-0051, Conventional Commits #182

docs: OEP-0051, Conventional Commits #182

nedbat commented Feb 1, 2021 •

edited

Loading

timmc-edx Feb 1, 2021

arbrandes Feb 1, 2021

nedbat Feb 2, 2021

nedbat Feb 3, 2021

kdmccormick Feb 8, 2021

nasthagiri Feb 12, 2021

kdmccormick Feb 12, 2021

davidjoy Feb 12, 2021 •

edited

Loading

symbolist Feb 15, 2021

feanil Feb 24, 2021

nasthagiri left a comment

nasthagiri Feb 1, 2021

nedbat Feb 2, 2021

nasthagiri Feb 8, 2021

nedbat Feb 9, 2021

symbolist Feb 11, 2021

nasthagiri Feb 12, 2021

feanil Feb 24, 2021

feanil left a comment

feanil Feb 24, 2021

feanil Feb 24, 2021

feanil Feb 24, 2021

kdmccormick Feb 25, 2021 •

edited

Loading

feanil Feb 26, 2021 •

edited

Loading

feanil Feb 24, 2021

feanil Feb 24, 2021

nedbat commented Feb 25, 2021

nedbat commented Feb 25, 2021

feanil left a comment

nedbat commented Feb 26, 2021

nedbat commented Feb 26, 2021

	* refactor: code reorganization that doesn't change behavior.
	* refactor: code reorganization that doesn't change behavior from the code consumer perspective.

	* fix: bug fixes.
	* fix: bug fixes including changes that remove or mitigate security vulnerabilities.

	We are not proposing any formalized footers in the commit message.
	We are not proposing any formalized footers in the commit message. Though the use of a ``BREAKING CHANGE:`` footer is encouraged as a way of providing more details about breaking changes.

docs: OEP-0051, Conventional Commits #182

docs: OEP-0051, Conventional Commits #182

Conversation

nedbat commented Feb 1, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidjoy Feb 12, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nasthagiri left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

feanil left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kdmccormick Feb 25, 2021 • edited Loading

Choose a reason for hiding this comment

feanil Feb 26, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nedbat commented Feb 25, 2021

nedbat commented Feb 25, 2021

feanil left a comment

Choose a reason for hiding this comment

nedbat commented Feb 26, 2021

nedbat commented Feb 26, 2021

nedbat commented Feb 1, 2021 •

edited

Loading

davidjoy Feb 12, 2021 •

edited

Loading

kdmccormick Feb 25, 2021 •

edited

Loading

feanil Feb 26, 2021 •

edited

Loading