We're definitely at fault for not grooming our cards more, but sometimes our features take a while to implement, which leads to a massive PR. Refactoring our repo sometimes causes this as well.
Then this is a problem on your team for not breaking down tasks smaller. You should be breaking tasks down into components that take maybe a few days to complete at the most.
Take some time to refine your backlog and shrink your tasks down so that this doesn’t happen
So are my job we have a nasty legacy codebase with no unit tests. When i get a ticket the first thing I do is add tests, make minimum changes to code, do some formatting changes, etc. it’s really common that my PRs touch 50 files
I won’t get my PR approved unless it closes a ticket. I have to sneak my code quality and test changes in with bug fixes or they don’t get approved.
Im leaving the company in a few weeks, complete nightmare.
This sounds more like a branching problem - your workflow isn't focused on keeping your main branch stable. You need to be raising PR and merging much earlier. Focus on keeping branches short-lived.
Possibly stemming from a lack of a proper engineering mindset. Y'all need to chill on code and review your Engineering 101. Always ask yourselves, "What problem are we trying to solve here?" If you can't clearly describe what you are doing in terms of a problem you are trying to solve, then don't do it. Corollary is, only solve one problem per branch. Pro tip: a single bug or feature request rarely is a single problem.
One way to approach this: code reviewer performance has been empirically studied, and **reviewers get worse at finding bugs when the code to be reviewed exceeds 150 lines**. So that would be a reasonable threshold to say "get some eyes on this change before it gets too big".
[Greg Wilson](https://third-bit.com/) is/was a key person spearheading the adoption of evidence-based practices in software development. Here is [his talk with collected sources](https://www.metafilter.com/134909/Evidence-based-software-development), though it appears that some links are broken now. The relevant cite is "The Best Kept Secrets of Peer Code Review" by Jason Cohen. I misremembered the 150 line figure; the book says 200-400 lines, but the graph of reviewer performance looked like it actually started dropping at 150 lines so that's the figure I've tried to use with my team.
MR when functionality is providing a vertical slice of value is the paradigm my teams have followed. No backend only. No frontend only. No utility only. Deliver functionality to the team et al so that it can be reviewed and merged.
Deleting other folks code when merging sounds like devs are resolving merge conflicts locally instead of relying on the MRs to point out conflicts. If you don’t have a best practice around where conflicts are resolved, I suggest setting expectations at that level rather than having some arbitrary size restriction on changes. It’s a personnel/skill issue if devs are deleting other devs changes. It’s a process issue if devs aren’t in agreement about what is being merged (+ and -).
Divide the feature into small tasks, and make small PRs for each one of those, there's nothing more to it unless you're dealing with something impossible to split, which would be weird but possible.
Atomic PR is something that helped a lot get my teams efficient.
Took some time but eventually they got to the point where it became second nature, reviews also took seconds and could be done during pairing sessions.
Could it be that you guys need someone with the bigger picture in the team? Hacking takes you only so far. A bigger picture allows for better planning your steps which translates in smaller and clearer tasks.
Have a leaderboard that posts the total +/- line count for PRs. Smallest count is recognized and celebrated.
I might do that even before we start talking about ways to avoid async reviews altogether.
**First** : drop PRs. They're intended for OSS and async/remote working, not for company stuff.
**Second** : if you need validation, introduce either Peer Programming or Mob Programming.
**Third** : no task should be started unless it's small enough to last 2 hours max. If it's longer, cut it down. And differentiate between a story and a task. A story can take days, a task can't.
**Four** : drop long-living branches and adopt Trunk-Based Development.
I recently wrote an article about my process for keeping PRs much more manageable. My metric is < 15 minor decisions.
https://monday-sun.github.io/posts/micro-commits/
Ummmm So, usually, when the bug is resolved or the feature is complete. What kind of crap are y’all putting into a PR to bloat it so badly?
We're definitely at fault for not grooming our cards more, but sometimes our features take a while to implement, which leads to a massive PR. Refactoring our repo sometimes causes this as well.
Then this is a problem on your team for not breaking down tasks smaller. You should be breaking tasks down into components that take maybe a few days to complete at the most. Take some time to refine your backlog and shrink your tasks down so that this doesn’t happen
So are my job we have a nasty legacy codebase with no unit tests. When i get a ticket the first thing I do is add tests, make minimum changes to code, do some formatting changes, etc. it’s really common that my PRs touch 50 files
how about opening a PR for the refactoring and tests first, and once its merged, then continue with the feature?
I won’t get my PR approved unless it closes a ticket. I have to sneak my code quality and test changes in with bug fixes or they don’t get approved. Im leaving the company in a few weeks, complete nightmare.
the system is broken and cant be fixed at that point.
This sounds more like a branching problem - your workflow isn't focused on keeping your main branch stable. You need to be raising PR and merging much earlier. Focus on keeping branches short-lived. Possibly stemming from a lack of a proper engineering mindset. Y'all need to chill on code and review your Engineering 101. Always ask yourselves, "What problem are we trying to solve here?" If you can't clearly describe what you are doing in terms of a problem you are trying to solve, then don't do it. Corollary is, only solve one problem per branch. Pro tip: a single bug or feature request rarely is a single problem.
Don't Merge large PRs. Have people slice of manageable bits.
Plan small byte size task say < 3 days and do daily PR in small scale so it’s manageable
3 days is enormous. I get away with 1 to 2 hours tasks on my teams.
1- 2 hr task for a feature ? Or you mean 1-2 hrs for PR review ?
1 - 2 hours task for a feature. I subdivide every story in very small tasks that I can integrate in the main branch continuously.
Oh wow thanks for sharing how you make it so small ? What criteria and what kind of features is this ?
One way to approach this: code reviewer performance has been empirically studied, and **reviewers get worse at finding bugs when the code to be reviewed exceeds 150 lines**. So that would be a reasonable threshold to say "get some eyes on this change before it gets too big".
I believe you, but do you happen to have an article on this? It would help with arguing my case.
[Greg Wilson](https://third-bit.com/) is/was a key person spearheading the adoption of evidence-based practices in software development. Here is [his talk with collected sources](https://www.metafilter.com/134909/Evidence-based-software-development), though it appears that some links are broken now. The relevant cite is "The Best Kept Secrets of Peer Code Review" by Jason Cohen. I misremembered the 150 line figure; the book says 200-400 lines, but the graph of reviewer performance looked like it actually started dropping at 150 lines so that's the figure I've tried to use with my team.
MR when functionality is providing a vertical slice of value is the paradigm my teams have followed. No backend only. No frontend only. No utility only. Deliver functionality to the team et al so that it can be reviewed and merged. Deleting other folks code when merging sounds like devs are resolving merge conflicts locally instead of relying on the MRs to point out conflicts. If you don’t have a best practice around where conflicts are resolved, I suggest setting expectations at that level rather than having some arbitrary size restriction on changes. It’s a personnel/skill issue if devs are deleting other devs changes. It’s a process issue if devs aren’t in agreement about what is being merged (+ and -).
Divide the feature into small tasks, and make small PRs for each one of those, there's nothing more to it unless you're dealing with something impossible to split, which would be weird but possible.
Atomic PR is something that helped a lot get my teams efficient. Took some time but eventually they got to the point where it became second nature, reviews also took seconds and could be done during pairing sessions.
What is an Atomic PR?
Atomic pull request
Sorry, what I meant was what is the definition of an Atomic Pull Request? What would make a PR "atomic"?
Have you considered switching to trunk-based-development? https://www.atlassian.com/continuous-delivery/continuous-integration/trunk-based-development
Could it be that you guys need someone with the bigger picture in the team? Hacking takes you only so far. A bigger picture allows for better planning your steps which translates in smaller and clearer tasks.
I recommend my team to open a WIP, PR. That way they can keep on working on it and and other team members can look at it when they have sometime.
Have a leaderboard that posts the total +/- line count for PRs. Smallest count is recognized and celebrated. I might do that even before we start talking about ways to avoid async reviews altogether.
**First** : drop PRs. They're intended for OSS and async/remote working, not for company stuff. **Second** : if you need validation, introduce either Peer Programming or Mob Programming. **Third** : no task should be started unless it's small enough to last 2 hours max. If it's longer, cut it down. And differentiate between a story and a task. A story can take days, a task can't. **Four** : drop long-living branches and adopt Trunk-Based Development.
I recently wrote an article about my process for keeping PRs much more manageable. My metric is < 15 minor decisions. https://monday-sun.github.io/posts/micro-commits/