Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

docs: OEP-0051, Conventional Commits #182

Merged
merged 3 commits into from
Feb 26, 2021
Merged

docs: OEP-0051, Conventional Commits #182

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
e2054a7
docs: first draft of conventional commits
Feb 1, 2021
9aae04b
docs: convert to oep-0051
Feb 24, 2021
af72356
docs: oep-0051: include more commentary
Feb 24, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
77 changes: 77 additions & 0 deletions oeps/conventional-commits.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,77 @@
####################
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

[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.

Conventional Commits
####################

Abstract
********

Commits should be clearly labeled for their effect on consumers of the repository. To do this, we adopt the `Conventional Commits`_ guidelines, which detail structured commit messages that clarify the impact of each commit.

This is part of our Change Transparency initiative.

Spec
****

We are adopting the `Conventional Commits`_ spec, with our own adjustments.

.. note:: Many people are familiar with the `Angular commit message format`_, which uses conventional commits and inspired the Conventional Commits spec. We are not adopting the Angular rules.

Commit messages have these parts::

<type>: <subject>

<body>

<footer>
Copy link

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?


Type
====

We use these commit types:
Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

"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?


* **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.
Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

@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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

@davidjoy davidjoy Feb 12, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

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.

  1. 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.

  2. 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.

Copy link

Choose a reason for hiding this comment

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

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):

Copy link
Contributor

Choose a reason for hiding this comment

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

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`

* **fix**: bug fixes.
Copy link
Contributor

Choose a reason for hiding this comment

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

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

* **perf**: performance improvement.
* **refactor**: code reorganization that doesn't change behavior.
Copy link
Contributor

Choose a reason for hiding this comment

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

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

* **revert**: undo a previous change. The previous commit message, including the type prefix, is included.
* **style**: improve the styling of the code.
* **test**: test-only changes. Adds missing tests or corrects existing tests. Tests accompanying other types of changes go into those commits.

Breaking changes include an exclamation point as part of the type::

feat!: removed the ability to author courses

Ideally, commits don't mix these types together. If they do, use the most important type in the commit message. The priority order for the types is generally: revert, feat, fix, perf, docs, test, build, refactor, style, chore.

Scope
=====

The `Conventional Commits`_ spec includes a parenthesized scope after the type. Open edX repos are large and varied, making standardization of scopes difficult. We won't use scopes. This could change in the future.

Subject
=======

Commit message subjects should be short. Put more information in the body of the commit message to fully explain your change.

Body
====

The subject of the commit is rarely enough information to fully understand the commit. The body can contain as much information as you like. Be generous. Take a moment to think about what you would want to know if someone else had authored this commit.


Footer
======

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

Choose a reason for hiding this comment

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

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.

Copy link
Member

@kdmccormick kdmccormick Feb 25, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
Contributor

@feanil feanil Feb 26, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

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



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.
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Seems like we should coordinate on that :)


.. _Conventional Commits: https://www.conventionalcommits.org
.. _Angular commit message format: https://github.com/angular/angular/blob/master/CONTRIBUTING.md#-commit-message-format