docs: Add conventional commit specification guildelines #11356

pedroerp · 2024-10-25T23:51:28Z

Summary:
Add guildelines to the CONTRIBUTING doc about PR titles and
description, with the purpose of improving our ability to classify commits and
navigate project history.

Fixes: #11039

Differential Revision: D65005411

facebook-github-bot · 2024-10-25T23:51:35Z

This pull request was exported from Phabricator. Differential Revision: D65005411

netlify · 2024-10-25T23:51:47Z

✅ Deploy Preview for meta-velox canceled.

Name	Link
🔨 Latest commit	`25c765a`
🔍 Latest deploy log	https://app.netlify.com/sites/meta-velox/deploys/672952aa5679840008a40217

…bator#11356) Summary: Add guildelines to the CONTRIBUTING doc about PR titles and description, with the purpose of improving our ability to classify commits and navigate project history. Fixes: facebookincubator#11039 Differential Revision: D65005411

facebook-github-bot · 2024-10-25T23:55:29Z

This pull request was exported from Phabricator. Differential Revision: D65005411

…bator#11356) Summary: Add guildelines to the CONTRIBUTING doc about PR titles and description, with the purpose of improving our ability to classify commits and navigate project history. Fixes: facebookincubator#11039 Differential Revision: D65005411

facebook-github-bot · 2024-10-25T23:56:47Z

This pull request was exported from Phabricator. Differential Revision: D65005411

…bator#11356) Summary: Add guildelines to the CONTRIBUTING doc about PR titles and description, with the purpose of improving our ability to classify commits and navigate project history. Fixes: facebookincubator#11039 Differential Revision: D65005411

facebook-github-bot · 2024-10-25T23:58:19Z

This pull request was exported from Phabricator. Differential Revision: D65005411

majetideepak · 2024-10-28T11:13:00Z

CONTRIBUTING.md

+The **PR body** must contain a summary of the change, focusing on the *what*
+and *why*. Keep columns at 80 characters.
+
+**Breaking API Changes.** A "BREAKING CHANGE:" footer must be added to PRs


Should we impose this now? We do not have a fixed public API yet, so users/reviewers will find it hard to determine this.

I'm planning to publish that External API docs we discussed a few weeks ago soon so we start socializing this more broadly. I feel like at least across the maintainers and reviewers group we should know what the main external API are (although the codebase organization does not yet reflect that).

A "BREAKING CHANGE:" footer must be added to PRs

Why footer? Should this not be the very first line in the PR description?

That's from the original conventional commit spec. An alternative is an exclamation mark after the type feat(type)!: .....

Why footer? Should this not be the very first line in the PR description?

The conventional commit spec's guidance is to add as a footer, presumably to make it easier to parse automatically when generating changelogs?

Like @assignUser mentioned, adding the "!" to the title may also be something we want to do, to make it super explicit that this commit has backwards breaking changes. Let me add this to the guidance here as well

CONTRIBUTING.md

majetideepak · 2024-10-28T11:15:27Z

CONTRIBUTING.md

+  * **docs** for enhancements to documentation (only).
+  * **refactor** for refactoring (no logic changes).
+
+* PR titles also take an *optional scope* field containing the area


Should we list some of the optional scope fields for usage consistency? Can we copy the github tags?

Good idea. What tags should we add? Here are some initial ideas:

vectors, types, expression-eval, operators, memory, dwio, parquet, dwrf, filesystem

Probably use shorter tags.
vec, type, expr, op, mem, dwio, dwrf, fs, parquet, func. Initially, we can keep this list flexible.
When a new tag needs to be added, we can update this list and keep it consistent.

Maybe we can have one additional one for (more generic) infrastructure or more specialized one for build or deps/dependencies? E.g. we could have a fix in a build script which would be fix(build) or fix(infra) or fix(deps)? Or something along the lines? I suppose this doesn't quite fall under the build type. Or it is something for the misc we discussed.

fix and feat are specific to changes that would require a bump of the patch or minor component of the version. So there could be rare cases where we have a feat(build): maybe adding a cmake config file or maybe removing a previously available option but usually cmake changes would be build: because they don't influence the actual API of the package (which ~= the way velox is consumed [in binary form].). Though 'in binary form' might only be accurate once we have a proper release, because currently everyone uses velox from source?

mbasmanova

@pedroerp I'm not a fan of conventional commits, but I don't have a strong opinion. Some minor comments below.

mbasmanova · 2024-10-29T13:26:06Z

CONTRIBUTING.md

+
+Commit messages that follow a strict pattern improve maintainability, and allow
+tasks such as summarizing changelogs and identifying API breaking changes to be
+automated. Despite requiring more rigor from authors and reviewers, consistency


consistency creates productivity

I believe I understand what you are trying to say here, but wonder if there is a better verb.

Rephrased it, please take another look.

mbasmanova · 2024-10-29T13:27:29Z

CONTRIBUTING.md

+where:
+
+* *Type* can be any of the following keywords:
+  * **feat** when new features are being added.


I feel this one violates 'do-not-abbreviate' guideline that I find useful. Can we drop this altogether and assume that if 'type' is missing then it is a new functionality? Or, change this to 'feature'?

Although I also don't like abbreviations in the code, the "feat" naming is part of the conventional commits spec and reasonably well known by folks engaged with other projects.

I think here, with the tension between abbreviation vs character limit, keeping this as "feat" for consistency may be a fair trade-off, but wonder what others think?

mbasmanova · 2024-10-29T13:27:42Z

CONTRIBUTING.md

+  * **feat** when new features are being added.
+  * **fix** for bug fixes.
+  * **build** for build or CI-related improvements.
+  * **docs** for enhancements to documentation (only).


Can we change this to 'doc' for consistency?

Curious, what should we use for PRs that do not fit any of these categories (say, adding tests) or PRs that have multiple of these?

For those chore is usually used but in the discussion it was pointed out that the word has negative connotations and was renamed to refactor. While I see that point I would agree that we need another bucket, maybe 'misc' or explicitly list them under refactor

We could also create a "tests" category.

... or PRs that have multiple of these?

I think in that case you would chose the "main" one. Like if you're adding a new feature with tests and docs, probably call out as a feature. But in general, if you have more than one "main" category, maybe you should have submitted them as different PRs in the first place?

Can we make docs -> doc?

mbasmanova · 2024-10-29T13:29:42Z

CONTRIBUTING.md

+
+* A *description* sentence summarizing the PR, written in imperative tone. The
+  description must be capitalized, not contain a period at the end, and be kept
+  under a 80 characters limit.


under a 80 characters limit.

I believe the whole commit title needs to be within that limit, not just the "Description" sentence.

"refactor(expression)" takes up 20 characters, 1/4 of the limit. Is this a concern?

Is this a concern?

I believe that's why conventional commit choose short words (chore vs refactor) or abbreviations like feat

Users can leverage the conventional commit words to keep the title succinct.
Add support for IPPREFIX type
vs.
feat(type): Add IPPREFIX

+1. They do add extra characters, but like @majetideepak pointed out, these are usually things you would have in the title anyway. For example:

Refactor expression code to prepare for feature X

could be:

refactor(expr): Prepare for feature X

I think in general abbreviating things that are very intuitive like expression -> expr should also be ok.

Btw, @mbasmanova, when you look at these title example individually, the value is not super clear, but if you have a list of hundreds of them already properly formatted (vs the free-form version) it makes a ton of difference.

Plus the benefit of automating the release notes and monthly updates.

mbasmanova · 2024-10-29T13:30:49Z

CONTRIBUTING.md

+
+Examples of PR titles are:
+
+* feature(type): Add support for IPPREFIX type


perhaps, a better example would be

feature(type): Add IPPREFIX type

"support for" is redundant and takes up space

A more succinct title would be to remove type again at the end.
feature(type): Add IPPREFIX

mbasmanova · 2024-10-29T13:31:57Z

CONTRIBUTING.md

+* refactor(vector): Use 'if constexpr' for Buffer::is_pod_like_v\<T\>
+
+The **PR body** must contain a summary of the change, focusing on the *what*
+and *why*. Keep columns at 80 characters.


Keep columns at 80 characters.

Would folks understand what this means? Usually, we do not write text in "columnar" manner. I used the term "wrap". Not sure if it is better though.

It would be nice to clarify why this is important. The reason I know of is to make the output of git show readable.

mbasmanova · 2024-10-29T13:33:29Z

CONTRIBUTING.md

+The **PR body** must contain a summary of the change, focusing on the *what*
+and *why*. Keep columns at 80 characters.
+
+**Breaking API Changes.** A "BREAKING CHANGE:" footer must be added to PRs


A "BREAKING CHANGE:" footer must be added to PRs

Why footer? Should this not be the very first line in the PR description?

mbasmanova · 2024-10-29T13:35:13Z

CONTRIBUTING.md

+that may break client builds, or semantic changes on the behavior of such APIs.
+
+If there is a Github Issue or Discussion related to the PR, authors must also add a
+"Fixes: <link>" section to the bottom of the PR body.


There are multiple syntaxes that work. I use Fixes #

I don't have a link, but I'm sure GitHub docs list all supported combinations somewhere. The point of doing this is to allow GitHub auto-close issues when PRs are merged. That said, one needs to be careful to not use this for PRs that partially fix an issue. For these I use "Part of #" to allow GitHub to link PR to issue, but not auto-close the issue.

Found the docs here:

https://docs.github.com/en/issues/tracking-your-work-with-issues/using-issues/linking-a-pull-request-to-an-issue

Looks like there are multiple keywords that work (including "fixes"), but "part of" is not listed in any of the docs. I suppose this is because github ends up linking the PR to the Issue since the Issue was mentioned (but does not automatically close the associated Issue).

But +1 on adding guidelines for "Fixes" and "Part of", depending on what authors want the semantic to be; will update the file.

mbasmanova · 2024-10-29T13:36:07Z

CONTRIBUTING.md

+If there is a Github Issue or Discussion related to the PR, authors must also add a
+"Fixes: <link>" section to the bottom of the PR body.
+
+To learn more about how to write great commit messages, check out these resources:


Wondering if there is a point in making a bit stronger request to all contributors to take the time and read these resources.

Changed the phrasing.

assignUser · 2024-10-30T00:02:12Z

CONTRIBUTING.md

+  * **feat** when new features are being added.
+  * **fix** for bug fixes.
+  * **build** for build or CI-related improvements.
+  * **docs** for enhancements to documentation (only).


For those chore is usually used but in the discussion it was pointed out that the word has negative connotations and was renamed to refactor. While I see that point I would agree that we need another bucket, maybe 'misc' or explicitly list them under refactor

assignUser · 2024-10-30T00:04:08Z

CONTRIBUTING.md

+
+* A *description* sentence summarizing the PR, written in imperative tone. The
+  description must be capitalized, not contain a period at the end, and be kept
+  under a 80 characters limit.


Is this a concern?

I believe that's why conventional commit choose short words (chore vs refactor) or abbreviations like feat

assignUser · 2024-10-30T00:08:05Z

CONTRIBUTING.md

+The **PR body** must contain a summary of the change, focusing on the *what*
+and *why*. Keep columns at 80 characters.
+
+**Breaking API Changes.** A "BREAKING CHANGE:" footer must be added to PRs


That's from the original conventional commit spec. An alternative is an exclamation mark after the type feat(type)!: .....

CONTRIBUTING.md

assignUser · 2024-10-30T00:13:29Z

CONTRIBUTING.md

+* fix: Prevent unnecessary flatmap to map conversion
+* refactor(vector): Use 'if constexpr' for Buffer::is_pod_like_v\<T\>
+
+The **PR body** must contain a summary of the change, focusing on the *what*


I think a bullet point list can work for smaller changes and should be used as an additional, quickly skim-able summary for larger PRs.

You mean we should add some guidance for authors to encourage them to describe the changes in their PRs as bullet point lists?

Yes for short PRs just bullet points and for longer PRs that have multiple paragraphs in the description an additional bullet point summary at the top. Though from seeing PRs that had good descriptions that seems to come naturally so we can add that later as it's 'just' for human readability.

czentgr · 2024-11-04T15:32:55Z

CONTRIBUTING.md

+
+**PR titles** must follow the pattern:
+
+> \<type\>[optional scope]: \<description\>


Nit: With the parenthesis around the optional scope - would the pseudo syntax be more

\<type\>[(<scope>)]: \<description\>

with [ and ] denoting the optional?

…bator#11356) Summary: Add guildelines to the CONTRIBUTING doc about PR titles and description, with the purpose of improving our ability to classify commits and navigate project history. Fixes: facebookincubator#11039 Reviewed By: xiaoxmeng Differential Revision: D65005411

facebook-github-bot · 2024-11-04T23:03:44Z

This pull request was exported from Phabricator. Differential Revision: D65005411

pedroerp · 2024-11-04T23:03:58Z

@mbasmanova @assignUser @majetideepak @czentgr addressed review comments. Please take another look when you have a moment.

assignUser

🎉

assignUser · 2024-11-07T17:31:16Z

CONTRIBUTING.md

+* fix: Prevent unnecessary flatmap to map conversion
+* refactor(vector): Use 'if constexpr' for Buffer::is_pod_like_v\<T\>
+
+The **PR body** must contain a summary of the change, focusing on the *what*


Yes for short PRs just bullet points and for longer PRs that have multiple paragraphs in the description an additional bullet point summary at the top. Though from seeing PRs that had good descriptions that seems to come naturally so we can add that later as it's 'just' for human readability.

majetideepak

@pedroerp Looks nice! I have one comment.

majetideepak · 2024-11-08T10:49:33Z

CONTRIBUTING.md

+  * **feat** when new features are being added.
+  * **fix** for bug fixes.
+  * **build** for build or CI-related improvements.
+  * **docs** for enhancements to documentation (only).


Can we make docs -> doc?

facebook-github-bot · 2024-11-12T18:23:58Z

This pull request has been merged in 11b6de1.

conbench-facebook · 2024-11-12T18:59:09Z

Conbench analyzed the 1 benchmark run on commit 11b6de18.

There were no benchmark performance regressions. 🎉

The full Conbench report has more details.

Summary: Check the PR title against cc pattern added in #11356. Fails with a pointer to CONTRIBUTING.md. We are running the title change on every commit as it takes under 5 seconds to run and this way we avoid weird states with skipped checks overriding failed results when the title was triggered outside of a commit. We could add a comment to the PR but that would require a workflow with elevated permissions using `pull_request_target` which should be avoided if possible. So I think the marginally lower ux is justified, we still get a marker/reminder for reviewers through this job. Pull Request resolved: #11520 Reviewed By: pedroerp Differential Revision: D65947963 Pulled By: Yuhta fbshipit-source-id: 290b97f784011367d3e481ff49f42b7da65fdf7a

facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Oct 25, 2024

facebook-github-bot added the fb-exported label Oct 25, 2024

pedroerp requested a review from mbasmanova October 25, 2024 23:51

pedroerp requested review from majetideepak and assignUser October 25, 2024 23:51

pedroerp force-pushed the export-D65005411 branch from 73d84b3 to f4e324a Compare October 25, 2024 23:55

pedroerp force-pushed the export-D65005411 branch from f4e324a to 15c75eb Compare October 25, 2024 23:56

pedroerp force-pushed the export-D65005411 branch from 15c75eb to 7eef00b Compare October 25, 2024 23:58

majetideepak reviewed Oct 28, 2024

View reviewed changes

mbasmanova reviewed Oct 29, 2024

View reviewed changes

assignUser reviewed Oct 30, 2024

View reviewed changes

assignUser mentioned this pull request Oct 30, 2024

Check that PR title matches conventional commits. #11389

Open

czentgr reviewed Nov 4, 2024

View reviewed changes

pedroerp force-pushed the export-D65005411 branch from 7eef00b to 25c765a Compare November 4, 2024 23:03

assignUser approved these changes Nov 7, 2024

View reviewed changes

majetideepak approved these changes Nov 8, 2024

View reviewed changes

facebook-github-bot closed this in 11b6de1 Nov 12, 2024

facebook-github-bot added the Merged label Nov 12, 2024

assignUser mentioned this pull request Nov 13, 2024

build(ci): Add conventional commit title check #11520

Closed


		Examples of PR titles are:

		* feature(type): Add support for IPPREFIX type


		PR titles must follow the pattern:

		> \<type\>[optional scope]: \<description\>

docs: Add conventional commit specification guildelines #11356

docs: Add conventional commit specification guildelines #11356

Conversation

pedroerp commented Oct 25, 2024

facebook-github-bot commented Oct 25, 2024

netlify bot commented Oct 25, 2024 • edited Loading

✅ Deploy Preview for meta-velox canceled.

facebook-github-bot commented Oct 25, 2024

facebook-github-bot commented Oct 25, 2024

facebook-github-bot commented Oct 25, 2024

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mbasmanova 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

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

majetideepak Oct 30, 2024 • edited Loading

Choose a reason for hiding this comment

pedroerp Nov 4, 2024 • 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

facebook-github-bot commented Nov 4, 2024

pedroerp commented Nov 4, 2024

assignUser left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

majetideepak left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

facebook-github-bot commented Nov 12, 2024

conbench-facebook bot commented Nov 12, 2024

netlify bot commented Oct 25, 2024 •

edited

Loading

majetideepak Oct 30, 2024 •

edited

Loading

pedroerp Nov 4, 2024 •

edited

Loading