It’s been a while since the last time I wrote! A lot has happened, I got engaged for example (claps claps)! We seem to be at the end of our Coronavirus journey here in Israel, and it wasn’t a fun ride.
Today I want to write about something that has happened this week. A Clash of Titans if you will. It happened when someone had to enter someone else’s code.
Some background
We are working on a new project, with many microservices, databases, API and other fancy words. Sadly/Happily, one of the developers had to take a break from his work to go to a course. This course is three months long and the project is intended to end in the next 2-3 months, so we had to take the stuff she made to ourselves. We decided to divide the work between all of the rest. It’s not that we have the time, but what are you gonna do?
Entering the code
One of the microservices she worked on was an API to a small MongoDB, a REST API with 2 main routes and a bit of logic. I ask another developer in our team to continue the API and change some things according to new requirements.
When she finished, she put her branch up for a pull request, like G’d intended 🙂
Another developer, to review her PR, downloaded the repo to his computer and started giving some comments. Suddenly, he saw a possible bug in one of the files he was reviewing, when he went to the PR page to comment on that, he realized that code wasn’t changed on this PR. Meaning two things:
- That possible bug was missed in the previous PR (I did that PR, shame on me!)
- He couldn’t add his comment to that file, as it wasn’t in the PR!
What would you do in this situation?
On the one hand, there is a possible bug in the code. That can’t get merged with the main branch. PRs are done exactly for these things, to let other people see what you wrote, let a fresh pair of eyes see what you came up with to solve the problem and comment on that.
On the other hand, that code hasn’t changed since the previous PR, it wasn’t part of the Task this PR was dealing with. That was obviously missed in the previous PR (again, shame on me).
What he decided to do
After much consideration, he wrote his comment in the header of another file saying that that comment is about that other page.
Now comes the great question;
Does she need to fix it in this PR or not?
And here came the disagreement. Of course, the disagreement wasn’t whether the bug must be fixed or not, that was obvious (yes, it must be fixed). The question was when and how to fix it.
She thinks that, because she didn’t have any changes on that part of the code (it was the other API route), it mustn’t be fixed on this PR. She made this PR linked to a specific task, and that task was solved in this PR.
On his side, he says that because it is a possible bug, it can’t be merged with the main branch and as such, it must be fixed on this PR.
Which side are you on?
I offered a compromise
We agreed that when we do code reviews, all the code has to be reviewed, especially on microservices where there isn’t much code anyway.
Moreover, we agreed that comments to files not in the PR CAN be added to the PR.
Finally, we kind of agreed the best way to address them is to open a new Task/PBI in the backlog to be fixed in the next PR.
Of course, this method will work so long that the code is not in production, but we don’t have bugs on production anyway 😉
Something that started as a simple PR, with some simple comments, turned out to be not so simple. I wonder how much curious stuff will happen continuing our journey as developers.
No Comments