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.

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

OEP-0051 Conventional Commits #189

Merged
merged 7 commits into from
Mar 17, 2021
Merged

OEP-0051 Conventional Commits #189

merged 7 commits into from
Mar 17, 2021

Conversation

nedbat
Copy link
Contributor

@nedbat nedbat commented Feb 26, 2021

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.

Mostly, this just establishes the guideline, points at the spec, and
clarifies what we will and will not do.
@nedbat nedbat force-pushed the nedbat/conventional-commits branch from 68c5ee2 to 86e928b Compare February 26, 2021 18:24
oeps/oep-0051-bp-conventional-commits.rst Outdated Show resolved Hide resolved
oeps/oep-0051-bp-conventional-commits.rst Outdated Show resolved Hide resolved
oeps/oep-0051-bp-conventional-commits.rst Outdated Show resolved Hide resolved
Copy link
Member

@kdmccormick kdmccormick left a comment

Choose a reason for hiding this comment

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

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

Subject
-------

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

Choose a reason for hiding this comment

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

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?

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Including them at the end of the subject seems good.

oeps/oep-0051-bp-conventional-commits.rst Outdated Show resolved Hide resolved
oeps/oep-0051-bp-conventional-commits.rst Outdated Show resolved Hide resolved
oeps/oep-0051-bp-conventional-commits.rst Show resolved Hide resolved
@inventhouse
Copy link

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

nedbat commented Mar 9, 2021

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"

Copy link
Contributor

@pomegranited pomegranited left a comment

Choose a reason for hiding this comment

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

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

oeps/oep-0051-bp-conventional-commits.rst Outdated Show resolved Hide resolved
oeps/oep-0051-bp-conventional-commits.rst Outdated Show resolved Hide resolved
**breaking changes to features**: changing how a feature works is not a breaking change. For example, users are sent to a new experience instead of the old experience. This is not a breaking change. It should get a "feat" label, but not a "feat!" label.

**DEPR**: removing deprecated code likely is a breaking change.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.


* **build**: anything to do with building or releasing the repo: Makefile, tox.ini, Travis, Jenkins, GitHub actions, and so on.

* **chore**: a repetitive mechanical task such as updating requirements or translations.
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Absolutely, we'll be updating them.

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.

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

nedbat commented Mar 11, 2021

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

@samuelallan72
Copy link

@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
Copy link

inventhouse commented Mar 12, 2021

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

nedbat commented Mar 12, 2021

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

inventhouse commented Mar 12, 2021

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

@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
Copy link

inventhouse commented Mar 13, 2021

@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
Copy link

stvstnfrd commented Mar 13, 2021

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.


* **test**: test-only changes. Adds missing tests or corrects existing tests. Tests accompanying other types of changes go into those commits.

* **exp**: experimental or exploratory changes that are not meant to be permanent. Occasionally changes have to be pushed to GitHub or even production that are intended to be short-lived. For example, debugging continuous integration, or ad-hoc debugging on live systems.

Choose a reason for hiding this comment

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

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.

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 see your point. "temp" was my first choice. I will switch it.

Specification
============

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

Choose a reason for hiding this comment

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

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.

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 will take a stab at summarizing the differences.

@inventhouse
Copy link

inventhouse commented Mar 13, 2021

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

nedbat commented Mar 13, 2021

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

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


* **chore**: a repetitive mechanical task such as updating requirements or translations.

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

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Subject
-------

Commit message subjects should be short enough to fit on one line. We aren't putting a hard limit on character length, but 70 characters is a good time to turn your attention to the body of the commit message. Put more information in the body of the commit message to fully explain your change. Jira or GitHub issue numbers can be included at the end of the subject.
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link

@inventhouse inventhouse Mar 13, 2021

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Choose a reason for hiding this comment

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

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.

Copy link

@inventhouse inventhouse Mar 14, 2021

Choose a reason for hiding this comment

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

@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

Copy link

@inventhouse inventhouse Mar 15, 2021

Choose a reason for hiding this comment

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

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

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.

Copy link
Contributor Author

@nedbat nedbat Mar 15, 2021

Choose a reason for hiding this comment

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

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

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

Choose a reason for hiding this comment

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

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.

Choose a reason for hiding this comment

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

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


**DEPR**: removing deprecated code likely is a breaking change.

**pinned dependencies**: updating the version of a pinned dependency seems like just a chore, but consider the repo consumer's perspective. If an updated dependency adds a feature, then the one-line commit to update the pinned version should be marked "feat".
Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member

@OmarIthawi OmarIthawi left a comment

Choose a reason for hiding this comment

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

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

Copy link
Contributor

@nasthagiri nasthagiri left a comment

Choose a reason for hiding this comment

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

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


feat!: remove the ability to author courses

Ideally, a single commit won't mix these types together. If a commit does, 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, exp.
Copy link
Contributor

Choose a reason for hiding this comment

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

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Subject
-------

Commit message subjects should be short enough to fit on one line. We aren't putting a hard limit on character length, but 70 characters is a good time to turn your attention to the body of the commit message. Put more information in the body of the commit message to fully explain your change. Jira or GitHub issue numbers can be included at the end of the subject.
Copy link
Contributor

Choose a reason for hiding this comment

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

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

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

Choose a reason for hiding this comment

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

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.

**breaking changes to features**: changing how a feature works is not a breaking change. For example, users are sent to a new experience instead of the old experience. This is not a breaking change. It should get a "feat" label, but not a "feat!" label.

**DEPR**: removing deprecated code likely is a breaking change.

Copy link
Contributor

Choose a reason for hiding this comment

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

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?


**DEPR**: removing deprecated code likely is a breaking change.

**pinned dependencies**: updating the version of a pinned dependency seems like just a chore, but consider the repo consumer's perspective. If an updated dependency adds a feature, then the one-line commit to update the pinned version should be marked "feat".
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants