4
bnjns
5y

How do you guys deal with PRs where things just don't go in and you're always making the same comments and suggestions?

We have a fairly experienced guy in the team who doesn't seem that familiar with the language (Kotlin), despite having now used it for almost a year. We're constantly making the same comments on code not using the correct syntax (basic things like val vs var) or following the style guide and after a lot of grumbling the changes are made, but the same issues are present in the next PR.

He also keeps doing things their own way, even if as a team we've reached consensus on a particular design pattern to follow, or way to solve a problem. When you mention this, you just get a "Hmm okay" but nothing changes. It's like things just go in one ear and out the other.

Even as the reviewer this is really frustrating and demoralising. PRs have loads of comments which makes you feel like you're being picky, and they take forever to get approved and merged.

I even often find myself effectively feeling bullied into approving once most of the main comments are addressed, because you're talking the brick wall that isn't yielding - and none of us are happy with the quality of the code going in. A couple of us are even starting to think "I'm just going to have to accept this and then fix it myself later", which is just not a healthy approach.

Now I'm blessed with an amazing manager who is well aware of the problem and knows from his own experience that this guy is genuinely problematic to work with. We're working towards a solution but I was wondering if anyone here has had a similar experience and how you worked towards solving it?

I'm a little at my wit's end :/

Comments
  • 10
    Use sonarqube style checking or similar and integrate it into your process. Run it as a pre-build step when the update githook fires on PRs. If it fails, fail the PR, dump the output of the stylechecker.

    Let him argue with a machine.
  • 2
    @SortOfTested Yeah, another guy has argued to use sonarqube for the same reason (as well as the other benefits it brings). Might be time to actually add it
  • 1
    @bnjns
    If something is important enough to be an actual standard, it's important enough to codify via tooling.
  • 0
    @SortOfTested That's a really good point. We did have a kind of linter for a while to try and solve the problem, but it wasn't very good
  • 1
    For general approach / style stuff that can't easily be automated, I've been known to send a quick message when a review is requested saying:

    "Before putting this in for review, can you please double check X, y and z - I know these have been frequent issues in the past, so best to check they're sorted before review this time!"

    Usually works.
  • 1
    PIP = Performance Improvement Plan.

    If the guy has goals that include working with the team, and checking in code that is up to scratch with the practices you agreed to as a team, yet is struggling to deliver on that, it's a classical case that can be tackled using a PIP. Either he improves in a given time frame, or he's out…
  • 0
    Don't know about this inferior technology but C# has a way to make project build fail if it's not to up to the specification. And you can really customize anything, even treat empty lines as build errors or warnings. So something like this must exist also.
  • 1
    @DanijelH Inferior technology? Someone fancies themselves as a C# fanboy 😂
  • 0
    @AlmondSauce It was a joke. But seriously, there has to be a way to make your codebase "team like" without human intervention.
Add Comment