5

I'm having an issue with a junior developer bikeshedding PRs. They are the primary approver for one repository. Things that should be up for an hour or minutes on PR spend days sometimes with back-and-forth comments.

An example from yesterday: I added an artifact to our build system. One line change. Get three back-and-forth comments asking why another thing is there. This is something that has existed for two months and is unrelated to my code change.

The other common set is needing to guide them down the path of a non-initial solution. You know how it is. There is problem Alpha. Initial thought is Solution A. Deeper thinking reveals it won't work because of Problem Zeta. So Solution B is needed. Put up a PR for Solution B, detail why it is implemented that way. First comment, "why not Solution A?" Have to guide them through the rabbit hole of how Solution A can't work because of Problem Z. They realize that and agree.

Socially, the junior developer is friendly and we get along wonderfully. But this blocking behaviour is impeding a lot of work (I've had three weeks of PRs backlog'd waiting for other PRs to get in).

When I asked him to please stop bikeshedding PRs, he started bikeshedding the definition of bikeshedding.

  • 1
    I'm voting to close this question as off-topic because even though the problem happens while Software Engineering, it is not a SE problem. It is a Interpersonal Skills or The Workplace problem. – Jörg W Mittag Aug 17 '19 at 13:09
  • 1
    Part 2 is definitely Software Engineering. And I'd say how to do useful code reviews is also part of Software Engineering. – gnasher729 Aug 17 '19 at 13:16
  • 3
    "Have to guide them through the rabbit hole of how Solution A can't work because of Problem Z. They realize that and agree." - perhaps you simply haven't accepted the need to explain the workings of the system, and thus begrudge the time necessary to do it? Perhaps even, it is a sign that the system is starting to exceed a level of complexity where a proper understanding of it can be economically reproduced? In only a number of years, the junior is going to be a senior who must understand how the system works if they are to be able to perform their jobs. – Steve Aug 17 '19 at 14:51
  • 2
    Honestly, these things don't sound like bikeshedding to me. They sound like ... code reviews. – Eric King Aug 17 '19 at 16:01
  • 1
    @EricKing: There's a difference between code reviews and rampant pedantry. One of them is productive, the other is not. – Robert Harvey Aug 17 '19 at 18:42
8

You have two different problems here.

One is: You make a change. Reviewer reviews it. Reviewer spots code that may or may not be objectionable near your change, and comments on it, possibly requesting a change.

The answer to that is an absolutely steely reply that this code has nothing to do with the change you made, is not something that he should review during your review, that it is outside the scope of your change, and that you will most definitely not change it. If he doesn't like it, then he or she can put a change request into the system with his name on it, which will be accepted or not accepted by the person responsible for it, and if accepted and when prioritised ahead of any other work, it will be given to someone to change. Before that, you are not touching it, and you are not discussing it.

Obviously you write this once and copy+paste it when needed.

For the second problem: If there is a solution A to a problem which is obvious but doesn't actually work, then it is good practice to add a comment at the start of your code for solution B that explains this. It's not just today's reviewer, it is also me looking at the code next year, or yourself looking at the code next year, who will be wondering "why didn't he use the much simpler method A?". Especially if it took a while to find that A doesn't work - you might actually have spent two days implementing A, and then figured out it doesn't work and thrown it away.

So add the comment to your code. At that point, the reviewer can see your code, and you can expect enough competence to understand it. It is not your job to explain it.

2

The junior programmer might just want to be heard or is having a hard time understanding your code or previous code. I guess they have been working on that repo longer than you. The development might be fast and PR quick if you asked their opinion before starting the implementation. That way as senior you will be including the junior in the implementation decision making. I find that discussing a problem and solution before implementing it increases the team cohesion and speeds up PR reviews.

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