12 Comments
Feb 12, 2021Liked by Kent Beck

This reminds me of Arlo’s commit notation: https://github.com/RefactoringCombos/ArlosCommitNotation

Expand full comment
Jul 5, 2023Liked by Kent Beck

This is part of the essence of go/small-cls https://google.github.io/eng-practices/review/developer/small-cls.html one of the most foundational engineering practices at Google.

Expand full comment

I have worked with groups that shun (or at least resist) structural change PRs because they view them as unnecessary or speculative or something else wasteful. They only grudgingly accept structural change PR X because Very Important behavior change PR Y depends on it and so they are forced to.

I haven't experienced this yet, but I worry that such a group would reject a structural change PR intended for "cleaning up before moving on", because the behavior change PR that it follows contains "the good stuff", so they view the following structural change PR as wasteful.

These suggest to me to _combine_ structural and behavior change PRs, rather than split them, but perhaps to reorder the commits so that behavior changes gather in the middle and structural changes happen before and after.

Maybe. I'm not sure.

Expand full comment
Feb 2, 2022Liked by Kent Beck

A long time ago, the Google C++ style guide had a rule that said "Separate functional changes from editorial changes", which echoes this sentiment nicely. Like other commenters, I feel that this applies equally, if not more, to commits. I like to mix refactor commits leading up to a functional change, and that change as a final commit in the same PR, because it provides motivation for the refactors.

Expand full comment

So separate make it work from make it right. It is useful also for commit logic.

Expand full comment

For me that is one of the strongest thing I found as driver for overall awareness and mastering.

That's why your first Tidy First? short conference really got me into it. You named what I was looking for.

I feel it is practical, unambiguous, force to think about what your change is doing and exposes reality of development without having to hide the refactoring.

It will educate developers with a simple enough rule which still leaves place for a learning phase.

My context of discussion for the rest is based on working on a large open source project with many different interests, with hardware support, networking, specific usage libraries from hobbyist, to researchers and dedicated engineers.

It's known for years that "commits must be atomic and do only one thing" and also all be all working. But I found it was hard to have a large crowd of different interests developers stick to it.

For real practical reasons, it's hard to get all commits tested locally and also by CI in some cases, online contribution tools make it hard to review by commits, not everybody is fluent with commit editing during a rebase with `git add -p` and `git reset -p`, here the rule is a bit larger so leave place for still dirty commits (learning phase). And also, creating these intermediate commits can require temporarily adding code which can be hard to justify if you are not convinced by the benefit. Also, people tend to adjust the definition of "atomic" depending on their current interest.

When both type of changes would be mixed in a PR, not everybody would do the effort of showing there refactoring are without regression or even know about it and the new features may be based on now wrong assumptions. This would lead to kind of bike shedding situations where the complains would not even be on the new feature at all or where the new feature is not wanted but some refactoring would be welcomed.

The worst being "but this new feature is great, we do not care about the rest" and regressions being merged because, yeah someone want the new thing, and why are you complaining about this weird case when we really want that new feature. The decision is about all or nothing.

Without a strong testing culture and infrastructure in place, it would not even be noticed that regressions are introduced. When a PR does both not changing anything and adding things, how can you test it?

If a PR changes "nothing", the way of demonstrating it is showing that it is somehow bit per bit or that all observables are identical. And so merging can be really really quick.

If you change one thing, you must show that the new things behaves as expected, but how can you at the same time, show that nothing changed in the previous commits?

As you emphasize, the review mindset is completely different. But not everybody notices in the first place. After having done the two different types of reviews, it would teach devs that it must be designed and implemented differently.

It exposes refactoring. When you have managers looking behind your shoulder, they would wonder why you spend time on refactoring instead of the new thing, so it would be always piggy backed on other features. It would be the opposite case as the case I talked about just before, it's not a byproduct, but it's the trick to make it merged.

If all the refactoring/structural changes must all be done outside, they become visible and part of the normal flow. Maybe it could then be argued that you spent too much time with refactoring, but it's already normal that some occur, maybe just not that much. You go from 0 being allowed to 30%. The education process is happening.

After this, you create a team that is aware of the different types, thinks that refactoring is part of development and value the effort, is forced to think about making non breaking changes for the refactoring (and maybe then doing it too for the new features), learned about splitting are re-organizing commits, working incrementally.

All this learning power, behind a really simple and hard to argue against rule.

Expand full comment

This could clearly be one of the steps that describe the path from PR => Code Review (or trunk-based development with "short-lived branches") to single-branch with limbo.

Now, the main challenge is that the Scouts' Rule of "Always leave the campground cleaner than you found it" might be neglected even more due to the burden of switching branches and creating distinct PRs. Developers can be less inclined to tidy up. On the opposite side, this can also produce too many PRs which would cause review congestion if the review process is present and not fast.

What do you think of the complementary approach where behavior changes are first hidden behind a "wip" feature toggle meaning that some changes are structural (with a final intent of behavior change) and the final commit or PR is disabling the toggle somewhat like this: https://github.com/jscutlery/semver/commit/14a9b3caf9288b4eac04602f3cc1d4917cbf9020

Nice find @Milosz. Conventional commits can also help https://www.conventionalcommits.org/en/v1.0.0/

Expand full comment