Welcome to Programmer Humor!
This is a place where you can post jokes, memes, humor, etc. related to programming!
For sharing awful code theres also Programming Horror.
Love it when my coworkers reformat the code style, making it nigh impossible to understand what they actually changed, while greatly inflating their “contribution.”
It also blows away the git blame, making it hard to know who actually changed that one critical line of business logic 3 years ago that you need to understand before trying to fix some obscure bug.
I have one coworker who does this constantly and if you just looked at git blame, you’d think he wrote the entire code base himself.
First things first: Your team needs a coding style.
Also: With git reflog ignore-revs you can filter commits that only adapt the style.
And while we're at it, check out the -C -C -C flag for git blame. https://git-scm.com/docs/git-blame#Documentation/git-blame.txt--Cltnumgt
Net removal of 1500 LoC...
I'm gonna make you break this up into multiple PRs before reviewing, but honestly, if your refactoring reduced the surface area by 20% I'm a happy man.
it's just removed unit tests that didn't work any more....
😱
lgtm
I try to keep my changes under 300-350 lines. Seems like a good threshold.
I'm still annoyed that Github doesn't have good support for stacked diffs. It's still not possible to say that one PR depends on a different one, and still has no ability to review and land them as a stack.
How is this different from creating a feature branch and making your PR against them until everything is done, then merging that into the main branch?
Making PRs against a feature branch still has the same problem.
Say you're working on a major new feature, like adding a new log in flow. It's a good idea to split it into many small changes: create initial log in form, implement user sessions, add SSO support, add "remember me" functionality, etc.
Those changes will likely depend on each other, for example adding the "remember me" checkbox requires the form to actually be built, and you probably don't want to wait for the reviewers to review one change before continuing your work. This means you naturally end up with PRs that depend on each other in a chain.
Stacked PRs (or stacked diffs, stacked MRs, whatever your company calls it) means that:
Making all your commits directly to a branch then creating a PR for the whole branch is similar, but reviews are a nightmare since it's only a single review for the entire branch, which can be thousands of lines of code.
At my workplace, we use feature flags (essentially if statements that can be toggled on or off) rather than feature branches. Everyone works directly from the main branch. We use continuous deployment which means landed code is quickly pushed to production. New features are hidden behind a feature flag until they're ready to roll out.
You can make a PR against your feature branch and have that reviewed. Then the final PR against your man branch is indeed huge, but all the changes have already been reviewed, so it's just LGTM and merge that bad boy!
You can make a PR against your feature branch and have that reviewed
But what if you have multiple PRs that depend on each other? Or are you saying only have one PR open at a time? That sounds like it'd be very slow?
I suppose it is possible to have two PR that have changes that depend on each other. In general this just requires refactoring... typically making a third PR removing the circular dependency.
It sounds like your policy is to keep PR around a long time, maybe? Generally we try to have ours merged within a few days, before bitrot sets in.
Sorry, my comment was unclear. I didn't mean a circular dependency, just PRs that have a chain of dependencies (e.g. PR 100 that depends on 99, that depends on 98, that depends on 97)
They're usually not around for a long time, but there can be relatively large chains if someone is quickly adding new features.
also iirc gitlab does offer something like this as a feature now with "merge trains" (though i've never really used it, usualy just go for the feature branch out of habit x) )
Please, no, I get flashbacks from my 6-month journey (still ongoing...) of the code review process I caused/did. Keeping PR scope contained and small is hard.
From this experience, I wish GitLab had a "Draft of Draft" to tell the reviewer what the quality of the pushed code is at: "NAK", "It maybe compiles", "The logic is broken" and "Missing 50% of the code", "This should be split into N PRs". This would allow openly co-develop, discuss, and steer the design, before moving to nitpicking on the naming, formatting, and/or documentation details of the code, which is likely to drastically change. Drafts do work for this, but the discussions can get uncomfortably long and convolute the actual finishing of the review process.
Once both reviewer(s) and the author agree on the code design, the "DraftDraft" could be collapsed into a link in an normal Draft to be mocked next. The scope of such draft would be limited by the earlier "DraftDraft".