Commit/PR process

[apcraig]

from @phil-blain,

I'd like to mention that it is really hard and demands a lot of energy to review big PRs like this, especially because we do not put a lot of emphasis on making small, atomic commits.

I think we could discuss this in the monthly meetings, but I would be really glad if we could make smaller commits, and make sure that each commit message explains why the changes are made, and make sure that each commits builds, etc. This would make it much more enjoyable (at least for me) to review PRs.

It would also open the possibility of using true merges instead of squash merges, which would also be very useful (in my opinion) because our history would be easier to read and understand. Just my 2 cents, as usual... :)

Here is a blog post discussing this subject that I like: https://github.blog/2022-06-30-write-better-commits-build-better-projects/

[apcraig]

I think there are trade offs. This would impose a relatively strict process to development that everyone would have to follow and that we'd have to try to enforce. But some folks are not as comfortable with git. And everyone tends to have their own way to do development, commits, and documentation of the commit. That would be heavily constrained.

The process currently oversees the PR, not the commit process. We want clean code, good documentation, well tested, etc. But we don't control the process for getting to that point. We use the squash merge to clean that mess up. Micro managing that could be time consuming and ultimately unsuccessful. There are valid processes that don't allow for small perfect commits. Things get implemented, refactored, reviewed, variable names change, and more during development of a PR. And I think that's fine. That commit history will be messy but is part of a reasonable process. The thing that matters is what's finally being PRed. And I prefer NOT to have all the commit history in our main repository.

I understand the benefit to doing smaller PRs. I'm probably more guilty than most of sneaking extra changes into PRs, like fixing random indentation and other formatting issues. I don't have a problem advocating for smaller PRs when it makes sense. And we can encourage things like good commit messages. But I don't think we should try to micromanage the development process and I think we should continue to do squash merges.

[apcraig]

We had a brief discussion at the CICE-Consortium meeting today. There was general agreement that PRs should be focused on a specific feature or model change, and combining things like multiple feature updates, code cleanup, and scripts revisions should be avoided and done in separate PRs. In addition, we agreed to try a new process where the PR template would be updated to request a detailed summary of all changes and that would be copied into the squash merge commit log. See #931.

[apcraig]

928 has additional PR documentation that can be used during the squash merge. Feel free to review that. If something is missing, maybe we should add some hints to the template files, so users know what to document in the PR.