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.
Perhaps I'll uncover more as I read more or you'll divulge more as you write more. Meantime, I worry about creating a longer chain of dependent events that looks on the surface like they "ought" to be independent. I can't shake this intuition that if PRs A, B, and C need to be accepted in the sequence A, B, then C, then perhaps we would benefit from combining them into a single PR. They seem to act collectively exactly like a single PR.
As I write these words, however, I can imagine that calling their attention to the structural PRs A and C as fundamentally different from behavior PR B has its own value, particularly as a means of building beneficial habits.
That's why I remain unsure. Maybe I'm closer to "not quite all in yet". :)
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.
Tidy First is a "make it right, then make it run" flow, which is okay because you already have something running. So I suppose you could look at it as the "make it right" from the previous "make it run".
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.
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
I tend to feel that the review is being congested by PRs adding new features that take too much time to review. Because for obvious reasons the new features must be discussed. And when this new feature is included in a thousands of line PR, the review gets mixed with guessing if each line either does not change anything or indeed change in the given direction.
If it can remove almost everything from the real feature PR, then it would sum-up to review only few changes. The other ones being transferred to "no need to review" PR that should be really quick to merge.
I like your idea of the feature toggle. It forces to think incrementally, with both the not the feature and the feature being present at the same time. And also, moves the hard discussion upfront and justifies adding unused functions as they would only be used later by the feature so not needed for some time.
On some large scale cases, I tried merging first a documentation change stating that the feature is going to be added incrementally and handling of both the old and new state. So all the discussion can be focused on the behavior only. Making all the following changes, a "update for the new feature" and not re-discussing with "does it make even sense". Which also required different people for the review.
This reminds me of Arlo’s commit notation: https://github.com/RefactoringCombos/ArlosCommitNotation
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.
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.
I can understand the scenario. I would still prefer to heal the organization rather than try to walk on a broken leg.
Perhaps I'll uncover more as I read more or you'll divulge more as you write more. Meantime, I worry about creating a longer chain of dependent events that looks on the surface like they "ought" to be independent. I can't shake this intuition that if PRs A, B, and C need to be accepted in the sequence A, B, then C, then perhaps we would benefit from combining them into a single PR. They seem to act collectively exactly like a single PR.
As I write these words, however, I can imagine that calling their attention to the structural PRs A and C as fundamentally different from behavior PR B has its own value, particularly as a means of building beneficial habits.
That's why I remain unsure. Maybe I'm closer to "not quite all in yet". :)
Thank you.
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.
That's exactly the Tidy First flow.
So separate make it work from make it right. It is useful also for commit logic.
Tidy First is a "make it right, then make it run" flow, which is okay because you already have something running. So I suppose you could look at it as the "make it right" from the previous "make it run".
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.
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/
I tend to feel that the review is being congested by PRs adding new features that take too much time to review. Because for obvious reasons the new features must be discussed. And when this new feature is included in a thousands of line PR, the review gets mixed with guessing if each line either does not change anything or indeed change in the given direction.
If it can remove almost everything from the real feature PR, then it would sum-up to review only few changes. The other ones being transferred to "no need to review" PR that should be really quick to merge.
I like your idea of the feature toggle. It forces to think incrementally, with both the not the feature and the feature being present at the same time. And also, moves the hard discussion upfront and justifies adding unused functions as they would only be used later by the feature so not needed for some time.
On some large scale cases, I tried merging first a documentation change stating that the feature is going to be added incrementally and handling of both the old and new state. So all the discussion can be focused on the behavior only. Making all the following changes, a "update for the new feature" and not re-discussing with "does it make even sense". Which also required different people for the review.