6

Often one wants to apply a code-formatter, like Black, or JuliaFormatter to a existing code-base. One where standards have tried to be followed but a few things might have slipped in. Once you have that you can insist that code matches the format and test that in CI etc.

But how do you get to that state? The direct solution is: you just do it. Apply the formatter. Make the PR. Merge it.

There are 2 key problems I am aware of if one just does it.

  1. This causes conflict for all other concurrent PRs. Which could be dozens or even hundreds.
  2. This complicates git blame. It is often useful to walk back through history of a given line and see where it was changed and why, but if the formatter touches say 30% of the lines suddenly you have to step over it and rework things out when tracking history of a line.

Perhaps one has to suffer through them but maybe there is some better way I am not thinking of.

Note this older SE question, which was proposed as a duplicate does not deal with the question of bringing a whole codebase with lots of pending pull requests under strict formatting conventions. This one here is about a scale that other questions did not take into account.

4

6 Answers 6

3

There is no way around the second problem, but anyone spelunking through history of code will in any case have to have a workflow to step past uninteresting commits one by one (rather than just run git blame on the latest version and stop), so the real problem is the first one (conflict for concurrent PRs).

If you can afford to write some additional tooling, and can bear with the code evolving into the desired style over time, the following is a system that can work (and which I've seen work). I've described a very gentle and gradual process, but you can omit or short-circuit several steps:

  1. Provide a linter or code tool that a PR author can choose to apply, which will (optionally) reformat only the changed lines from a PR. (Of course, it may end up touching a couple of surrounding lines, but basically the size of a single PR does not change much, and lines entirely unrelated to the PR aren't edited.)

  2. Have a few "beta-tester" contributors use this on a few of their PRs and get feedback from them to make sure it works well and is convenient to use, with no major bugs.

  3. At this point, announce that anyone can choose to apply this on their PR before sending it out for review. (They can also apply it to their under-review PRs, if the reviewer doesn't mind the additional review cycle.)

  4. (And if someone is brave or crazy or motivated enough to go ahead and apply it to an entire file, it's on them to deal with the other people who may be editing the same file. Point out that this is not recommended unless it is known or expected that no one else is working on the same file.)

  5. Start surfacing diffs that the tool would generate, privately to each author (a pre-push hook or whatever).

  6. Start surfacing diffs that the tool would generate, to both reviewer and author (on each PR). This will allow the reviewer to suggest changing the style of the affected lines.

  7. Start enforcing (in code) that changed lines on PRs should conform to the tool's style. Presumably everyone is already doing this by now by default, so the change is not disruptive.

  8. You could stop here: ancient code that no one touches will continue to be in the "wrong" style, but any code that people touch (including all new code) will surely be in the new style, and at any point, anyone so motivated can send out PRs fixing whatever code (functions or files or directories or…) in the incorrect style they encounter or care about.

  9. Periodically generate a report of the fraction of the codebase that is not yet in the new style. At some point, it will get small enough to make a "final push" of separate PRs you (or whoever is working on this effort) generate modifying the style, which should be reviewed like any other.

The biggest drawback with this approach, of course, is that code style can be inconsistent, but presumably that is already the case (and there may not be much of it, as you mentioned a codebase "where standards have tried to be followed but a few things might have slipped in"), but it does scale to large codebases.

1
  • This is a brilliant answer. It doesn't cause any conflicts that would not have already occurred. And using a GitHub action make review suggestions action, will do this naturally and unobtrusively. To only changes lines Aug 13, 2021 at 19:54
6
  1. This causes conflict for all other concurrent PRs. Which could be dozens or even hundreds.

This problem can be solved by a little thing we call "coordination", aka "talking to each other".

If you want to make large-scale changes like this, you will need to stop everything else you are doing. This might sound scary at first, but consider this: it only takes one second to check out the code, run the formatter, commit, and push it.

The important thing is to make sure that you don't get held up doing this. If you have code review procedures in place, make sure they don't get in the way.

You don't have to review the commit. Instead, what you do, is to beforehand review the formatter and its options. You need to convince yourself that running the formatter cannot possibly break anything. Review the heck out of it, run it in parallel for a couple of months (run the test suite on your current code, run the formatter, run the test suite again, do that for as long as you need to convince yourself on every single commit), ask fellow developers for their experiences, etc.

Make sure that the exact version and the exact settings for the formatter are part of the commit message of the commit that introduces the change. This allows every developer to reproduce the commit to ensure that you didn't sneak in any changes together with the formatting changes.

  1. This complicates git blame. It is often useful to walk back through history of a given line and see where it was changed and why, but if the formatter touches say 30% of the lines suddenly you have to step over it and rework things out when tracking history of a line.

This one, unfortunately, is not avoidable, unless you have some sort of semantic diff or semantic blame tool that understands the semantic structure of the code, and doesn't just stupidly treat it as a dumb array of characters.

5
  • 1
    Right, but coordination is no "little thing" when it comes to surficiently large projects. Checking one open source project I am involved in there are currently 80 active branches from core contributors. and I suspect (but can't easly check) 200+ from non-core contributors PRing from forks. It's no little thing to stop everything else; or even to talk to all non-core contributors. May 28, 2021 at 17:47
  • For what you want to do, everybody has to use the same coding style. That implies you have a communication channel that allows you to inform everybody of the coding style. Hence, you should be able to use the same communication channel to inform everybody of the date of the introduction of the coding style, no? If you don't have such a channel, you will simply end up in the same situation every couple of months anyway, because the coding style will diverge again. May 28, 2021 at 18:03
  • 1
    Having a ability to communicate 1-1 via pull request and CI error message does not imply the ability to broadcast. And CI error message can make certain to prevent style divergence. Nor does it imply ability to coordinate the wrap up of 300 active branches in timely fashion before new ones are openned. May 28, 2021 at 19:03
  • @LyndonWhite "Right, but coordination is no "little thing" when it comes to surficiently large projects." Sufficiently large project always need overhead management. It's a well documented fact that the amount of overhead required tends to exponentially (or at least more-than-linearly) increase as the size of a project increases. If you don't like having to manage a lot of things, then you shouldn't work with big projects. That's about the long and short of it. This is why we try to separate projects as much as we can, rather than pile it all together.
    – Flater
    May 30, 2021 at 21:49
  • It is possible to put automated tooling fixes in their own commit and list them in a file for git blame to ignore (using git config blame.ignoreRevsFile, [1])
    – Whymarrh
    Jun 3, 2021 at 19:42
3

I don't think there's a painless option. I think the way I would handle this would be to make a declaration that after a specific date, no PRs would be accepted if they don't meet the coding standard. At that date, you apply code formatting to the entire project. Anyone who can't get their PR in before that date will be required to rebase or merge.

You should make sure that you are decided on not only the coding standard but what tool(s) you will use to apply formatting and/or validate it. The specific tool should be part of the announcement so that contributors can apply it and expect consistent results. If possible, you should automate the code standard enforcement. No PR can commence if the linter balks.

What you should absolutely avoid is mixing real changes with formatting changes. When they are mixes, you can expect bugs and other undesirable changes to be accepted. It's too hard to differentiate between the non-functional changes and the formatting changes. In addition, any functional changes that do come with the formatting changes should be considered significant risks. It's really crucial to not mix them.

The big decision is how long do you give your contributors to get their current PRs done. If you have any statistics around how long PRs stay open, that might help guide your decision.

0

If you have formatted file x that I had checked out, then I can just apply the same formatting to file x myself. With my tools, I can see differences between files before creating a pull request. If I see x has 100 format changes + 10 changes that I made, then after using the formatting tool only my changes should remain. Assuming the formatting tool is deterministic. You wouldn’t want a non-deterministic tool.

0

You can't retile the entire office without getting in the way of the staff who works there. Similarly, you can't refactor the entire codebase without affecting active branches (when it's time to merge).

You could, for example, retile the office room by room, making sure there's no one in each room as you tile it. The real world analogy here is to run the code formatter selectively, based on what tasks are currently being worked on and which parts of the codebase are not being affected.

Depending on the size of your team and how many active tasks there are, this could be trivial, require some planning, or be completely impossible on a large scale.

Alternatively, you could also simply build a new retiled office, and tell the staff that when they finish what they're currently working on, they have to relocate to the new office. Then, when everyone is relocated, you go back to the old office, pick up the things that have been worked on (since the new office opened), and bring those to the new office.

In real world terms create a second branch. Run the code formatter across the board on that second branch. Test it, to be sure. From that point on, tell people to only branch new tasks from the second branch.
Everyone else keeps working on the existing tasks, finally merging them back into the old branch. When all pending branches have been merged, you re-merge the old master into the new master. It may cause some merge issues, but it should be relatively minimal compared to other approaches, and it wholly depends on how many active tasks needed to be finished (and how big those changesets were).

But again, company size matters. Small companies can do this with minimal friction, larger teams might be able to get by with some active management, but enterprise-grade teams are still liable to suffer through most of it.

Furthermore, it's generally more efficient to not have an excess of open branches. Less stuff to re-merge or keep up to date, less overhead effort. Not that it doesn't happen that you have to park a branch for an unexpected reason, but less is better, and more tends to mean more branch cleaning work (re-merging for updates, conflicts, ...).
So I intuitively suspect that the pain of doing this global refactor is going to correlate to how much you already run into branch juggling in your day to day workload. If you run a tight ship, the global refactor may be a storm easily weathered. If the ship is already a dingy to begin with, it's going to be rocking back and forth during the storm that is your refactoring.

0

Technically this is hardly a problem, solutions have been presented in other answers. The trouble with this will ultimately be social and organization related.

If you have people on the team who do not like the new rules and there is no recognized team lead who is behind the system, these people will continue to sabotage it and make up reasons why you should go back to "anything goes". It either needs to be clear this is the way forward and there will be no more debate about it or you must be confident that a critical mass is behind it. If neither is the case it will be pearls cast before swine and just cause a lot of fuzz with little outcome.

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.