InfiniTimeOrg / InfiniTime

Firmware for Pinetime smartwatch written in C++ and based on FreeRTOS
GNU General Public License v3.0
2.69k stars 921 forks source link

Format of commits #1516

Closed Riksu9000 closed 1 year ago

Riksu9000 commented 1 year ago

We currently don't have any requirements for commits. This leads to everyone having their own way of doing things, and making the commit title and description not being reliable.

I'd like to use this format from https://www.klipper3d.org/CONTRIBUTING.html#format-of-commit-messages

The "module" part gives a lot of context to the commit, and also forces the commit to be focused on a single "module". I don't know if we need the Signed off part.

The commit description explaining the commit is also very important. Sometimes I examine a commit and don't understand the purpose of it. The change was discussed in the PR, but there's nothing pointing me to the PR, so the description is lost.

Therefore I think we should be stricter about the commit descriptions, no matter how good the description is in the PR.

In #1514, I started this discussion by writing that commits fixing formatting from previous commits should be squashed. I'd like to expand this to include any commits that fix mistakes from previous commits.

Let me know what you think of these ideas.

Avamander commented 1 year ago

I'm thinking maybe it would be helpful to use squash merge more and add those descriptions then (by ourselves). Because yeah, more detailed commit messages would be quite helpful.

Some predefined commit title format would also be nice. Like begins with a capital letter, no [tags], doesn't end with a period - that type of stuff.

JF002 commented 1 year ago

I fully agree to this. The description and documentation of the PR are very important for the review process, but one should be able to understand the goal and purpose of any commit by reading the history and commit message. And, as @Riksu9000 mentioned, there's no way to link a commit to a specific comment or PR in Github so the commit should be self-explanatory.

The commit format suggested by Riksu seems very reasonnable (title, short description, more details). We can also take some inspiration in the contribution guidelines and commit conventions from the Micropython project. I especially like this part:

The MicroPython project adheres to a strict linear git history, with no merge commits. All PRs must rebase cleanly. Please prefer to break up your work into small commits. It's always easier to squash them later than to split them.

JF002 commented 1 year ago

I'm thinking maybe it would be helpful to use squash merge more and add those descriptions then (by ourselves). Because yeah, more detailed commit messages would be quite helpful.

I would use this technique as a last resort. Most of the time, it makes sense to break a feature in multiple commits. Let's take the integration of a new sensor : you'll first write the low-level driver, then a higher level class that use the driver to provide services to the application, then write an app that make use of data from the sensor. In this case, it would make sense to have at least 3 commits and not squash them into a single one. But yes, in case of commits that are mostly useless, squashing them and write a meaningful comment would make sense.

Riksu9000 commented 1 year ago

Commit requirements have been added to the maintainer's guide, but not yet to any of the contributing guides. I'm not sure where it should be put, in "how to contribute", "coding conventions" or somewhere else?

JF002 commented 1 year ago

I would add those rules in "how to contribute", since they are not directly related to the code.

Thinking about this... shouldn't we merge those 2 pages? "coding conventions" could be a sections inside "how to contribute". What do you think?