Improve our ability to classify PRs/commits

[pedroerp]

To improve our ability to classify PRs while summarizing monthly updates (and in the future, release changelogs), we are proposing two additions to our open source development workflow:

1. Adopting conventional commits:

Following on @assignUser 's proposal a while ago (#3330), conventional commits would help us differentiate new features from bug fixes, refactoring, documentation improvements, and so. Specifically, the proposal is to, moving forward, use the following pattern of commit title prefixes:

• "feat: " for new features being added
• "fix: " for bug fixes
• "build: " for build or CI-related improvements
• "docs: " for enhancements to documentation (only)
• "refactor: " for refactoring (no logic changes)

To further help classification, conventional commits may take an optional field containing the area of the code a PR is changing, such as "fix(expression): " or "refactor(parquet): " or "feat(memory): ". We should discuss a set of valid tags/components and document them in the contributing guide.

Example: #11017 would have been titled:

fix(function): Timestamp and interval arithmetic bug

2. Changelog and Breaking API Changes:

To further improve our ability to summarize changelogs (either monthly or when cutting future releases), we would like to ask authors to add a "Changelog: " section to the PR description, whenever new features are added. The Changelog would contain a sentence (one line or two) summarizing the feature added by the PR. This message would automatically get processed while generating changelogs.

Likewise, any PR containing changes to APIs (or their semantics) should also contain a "Breaking API Changes: " section in the PR description that explains the change. This will help us keep track and summarize breaking API changes for future releases.

--

As with any changes, it will take us some time to adapt. These changes would naturally be documented in our https://github.com/facebookincubator/velox/blob/main/CONTRIBUTING.md guide to alert new contributors, communicated to the community, and enforced by maintainers and code reviewers.

Please comment below if you are supportive of the changes or not (if not, please explain why).

[pedroerp]

Cc: @mbasmanova @oerling @xiaoxmeng @majetideepak @assignUser @Yuhta @kgpai @rui-mo

@bikramSingh91 @kagamiori @aditi-pandit @yingsu00 @JkSelf

Please help other folks who might have an opinion on this.

[majetideepak]

This is very helpful for project management. The Arrow RS https://github.com/apache/arrow-rs/ project has been following this.

[assignUser]

Thank you, very nice write up!
Just a nit: the commits don't have to scream at us :D fix: v FIX: but that's both inconsequential and subjective ^^

[majetideepak]

I am in favor of lowercase (fix). Uppercase (FIX) does seem to scream at us :).

[pedroerp]

lol, I just FIXED the proposal :)

[Yuhta]

For 1, I think it's ok to adopt it for a pure informational purpose; using it for versioning would quickly go out of control because people for sure will tag the things wrong, and a big FIX is often more significant (in terms of functionality/performance change and breaking compatibility) than a small FEAT, these tags should not be used as criteria to bump versions.

For 2, it's very hard to define breaking change now because we still don't have an API. The most explicit and safe way would be starting creating an api directory first and move the things that we should track compatibility there. Still for Meta internal usage the interface is much broader, so even a change is not changing API, it could break internal stuff, we need to be careful about it. Also behavioral/performance change should also be considered breaking sometimes; this is much harder to track than interface change. I don't have a good idea how to track these things yet.

For 3, it feels redundant because it can be quickly implied from the list of files changed. So I would say let's keep these for issues and not tagging PRs with these.

[Yuhta]

@majetideepak I would say API_CHANGE, PERFORMANCE_CHANGE, and BEHAVIOR_CHANGE are more important than FEAT, FIX, because the former would decide the versioning and help trouble shooting, while the later are only informational. So the former should be mandatory and later are optional.

[assignUser]

Just as a note: in general it might be difficult at the moment to mark breaking changes due to the lack of defined API but certain changes to obviously user exposed areas could still be marked breaking to help current users when they are updating their pinned sha/version of Velox. For example changes to the query plan api or types etc.

@kgpai nice idea rephrasing chore :)

[assignUser]

@pedroerp @kgpai thinking more about the rewording of chore I still agree with it but refactor seems not quite correct for things like CI or build system changes (i.e. updating actions versions). Maybe something neutral in both meaning and scope like task?

[pedroerp]

Could we use the "build" tag for these (build and CI related work) instead?

[assignUser]

Hm, that would normally use the other tags so fix(build) or feat(cmake). I'm fine with having a dedicated refactor tag but I think we do need a tag for miscellaneous changes not covered by the others and not relevant for versioning. Ah maybe misc that comes close to chore?

[majetideepak]

Breaking Changes:

We can be explicit and say Breaking API Changes:

[pedroerp]

Modified the proposal above.

[kgpai]

On 3 - Certain fundamental changes may affect many areas, In this case adding which areas affected might become inaccurate and probably not very useful.

[pedroerp]

Yes, these should be best effort. If there is nothing that matches the tags, I suppose it is ok to skip them. Hopefully most PR would match at least one of these tags though.

[pedroerp]

One open question is whether we use actual github tags to classify PRs, or the conventional commit pattern of adding it to the diff title, like "fix(memory): " or "refactor(expression): ". How does everyone feel about it?

@kgpai @Yuhta @majetideepak @assignUser @mbasmanova

[majetideepak]

I don't think GitHub tags get reflected in the final git commit.
We will need the conventional commit patterns of adding it to the title for it to be machine readable.

[assignUser]

Agreed, it should show up in the actual git commit. Otherwise the information might be lost (i.e. a tag is accidentally removed either on a PR or deleted all together).

[pedroerp]

Makes sense. I re-worded the proposal.

[JackyWoo]

Label maybe an alternative solution.

[assignUser]

Not really because that info can be lost: #11039 (reply in thread)

[assignUser]

Now implemented in CONTRIBUTING.md