Stele can you describe a sample comment? As a recently-minted tech lead I'm curious how the TL handled it in your case. When you say they talked to the person, do you mean 1:1? Personally I actively avoid doing that, but I wonder how others approach it.
I guess the question is “are their suggestions ever useful?”. I have one pedantic co-worker who also annoyingly works a 4 day week, remotely, time offset. When he ends up reviewing a patch it often turns into a multi week code review process. The problem is that once you get all the way through the process he’s generally right and the code is better for it.
If it’s useful, then great. If it’s not then you need to review your teams’ code review policy. I have a strong feeling that most simple/pedant things can be caught by linters/static analysers. Have those linters are part of your pre-merge workflow. Then the rule is don’t bother looking for those things. Code review should be about making sure the patch and commit message match. It does what it says, the logic looks reasonable and testing is in place. Everything else is a matter of personal taste and not relevant.
For instance, the PR this sprint there were 15 comments. 3 of them were useful (2 unnecessary lines I was doing in unit test by habit). 1 place I used a string builder that I could have passed a parameter.
6 of them were questioning things we had already done between 4 and 10 times, in 3 or 4 cases something they themselves approved last week, saying that I should change it for "convention" or some nonsense, even though there were 0 linter errors. One questioned the logic on a unit test that they themselves suggested. Those I pushed back on, and talked with the team lead about, and he said yes stick with the existing pattern, don't change it for one story. If all those instances are wrong we can meet and discuss and change all.
4 of them were mindless nitpicks that didn't matter but I changed them rather than arguing. Naming of CSS selector, class stuff. No error, no incorrect code, no bug, just this one person thinks it should be that way and arguing with them is exhausting.
The last 2 were actually wrong. One suggestion for a subscription I tried and it broke the functionality, so I left it the way I had it. The other was... The story asked me to put in placeholder pages for a new menu. By default a new Angular component creates a CSS class with it. The linter doesn't like empty CSS classes. I preemptively put a comment about ignoring that because placeholder, and if they are still empty in future stories on those pages, we can deal with it then. And they still asked me to delete them! 
Which is just insane because then if whoever does those stories needs CSS, it's creating extra work. Delete, then possibly re-add or just delete once, which may not even be needed.
What does your boss say?
Stele can you describe a sample comment? As a recently-minted tech lead I'm curious how the TL handled it in your case. When you say they talked to the person, do you mean 1:1? Personally I actively avoid doing that, but I wonder how others approach it.
Sample comments above.
They had a meeting after stand-up recently. I don't know details.
Part of the issue was we added a tech task to the backlog to install a linter, per their suggestion 6 weeks ago. It was something our lead had been meaning to do but until me and this other person got added a couple months ago, just hadn't had time.
But, instead of checking with the PO or scrum master about what task to do, this dev went and grabbed the linter task from the backlog one day and installed it 5 weeks back or so. Then they started pointing out linter errors on our PRs, even though it's not required yet.
Also we haven't gone over any of the rules. I know at least the one for arrays is wrong, goes against the Google Angular style guide. Others I haven't had time to investigate. But we're basically forced to follow what this one dev, who isn't the FE lead, or the team lead, decided on a whim to install and force on us.
Meanwhile, a couple sprints ago I had 2 bugs in my name, while they had 7. They also commented out some code and said "why is this here?" last week. I read through the code and said hey that's there to prevent this. I think it's going to create a new bug. They proceeded anyway because that was a draft PR but I went and tested locally and confirmed it created a bug. Then I commented on the final PR "tested and this does cause a bug". So I saved them from causing another bug by messing with code that wasn't even related to their story.
Seems to me like they need to worry about making their own code bug free before mucking around in reviews.
But I'm possibly not being objective because they are annoying the shit out of me. So, that's why I'm asking... 10 years of dev work and I've never had to deal with anyone like this before. Maybe I've been lucky.
It feels like they're trying to justify their employment by looking like they're doing a bunch of work when in fact they are just wasting everyone's time.
What does your boss say?
"Hey guys, let's use Angular."
(sorry, couldn't resist)
To me, the linter thing sounds like it exacerbates everything else, and since it's a tech issue it might be a lot easier to push forwards.
Because in teams I've worked with, the linter (and auto-formatter) is approximately the dividing line between what is and isn't left to preference. E.g.: some people I work with feel strongly that all booleans should be named isFoo or hasFoo, never just foo. At some point there was a proposal to add a linter rule for this, but it wound up not happening. Now as a result, certain people sometimes suggest renaming booleans in code reviews, but everyone understands that the PR author is free to decline, because it's not a rule the team has adopted - if it was, it'd be linted.
So I guess if it was me I'd push on the task for getting linting in place (tbh I have no idea how a team works without it). And secondly make sure that the TL understands that linting config must be checked into the repo, it's not something where each dev can have their own favorite setup.
Possibly a stupid question here. Is it possible to add a linter (and maybe an auto-formatter) to MSVS for Python? and are there "good practices" configs out there?
I'm writing some code for a couple of hobby projects - not to mention AoC in December - and would love to produce some "industry standard" code rather than the hodgepodge of naming conventions I'm doing right now.
I use Visual Studio Code with black and flake8 (considering switching to ruff). There are tons of extensions that implement lots of formatting and best-practices type stuff. I’ve never used MSVS for Python.
What does your boss say?
Well boss pointed out the bug discrepancy, so he's paying attention. When I asked if it was just me being sensitive or something or of he had talked to team lead, he said no, he was aware. So, yeah...
But it didn't seem to be improving.
It feels like they're trying to justify their employment by looking like they're doing a bunch of work when in fact they are just wasting everyone's time.
I've certainly had that thought. The 3 newest team members are all contractors probably hoping to get hired. But if they are trying to be more useful than me they could start by not checking in so many bugs.
To me, the linter thing sounds like it exacerbates everything else, and since it's a tech issue it might be a lot easier to push forwards.
Because in teams I've worked with, the linter (and auto-formatter) is approximately the dividing line between what is and isn't left to preference. E.g.: some people I work with feel strongly that all booleans should be named isFoo or hasFoo, never just foo. At some point there was a proposal to add a linter rule for this, but it wound up not happening. Now as a result, certain people sometimes suggest renaming booleans in code reviews, but everyone understands that the PR author is free to decline, because it's not a rule the team has adopted - if it was, it'd be linted.
So I guess if it was me I'd push on the task for getting linting in place (tbh I have no idea how a team works without it). And secondly make sure that the TL understands that linting config must be checked into the repo, it's not something where each dev can have their own favorite setup.
Yeah it's already checked in, but it's not required for builds yet. Because it hasn't really been configured to team specs since this dev just went and did it all by themselves. Until they happens they need to chill out on errors and that's what TL told me was stressed in that meeting, but it's still happening, like with the placeholder files.
And the rest wasn't even linting stuff, just pedantic crap. For one, they are hung up on never using object for a type, ever. Now, the component library we are using has documentation, examples, forums, etc. and they use object for data grid validation rules and toolbar buttons in all of those places. So I'm following the docs. And this is like the 4th table, and we've went round and round and so they have approved it twice before but then protested this week. When I refered to the docs and previous reviews they just came back with "this should be an easy fix". So I replied "matches documentation, not an error, and used on 4 tables already approved". So they finally resolve it with "don't want to go round and round about simple stuff".
And I'm thinking WTF. Every damn comment is simple stuff. It's not that it would take me long to fix it, it's that it's not wrong. Takes more time to deal with their bullshit than change it, which is why I relented on the CSS classes. But damn I have to draw a line somewhere.
The other fun one is the suggestion I tried that broke my code. They said "x is valid but y is preferred". Like whatever, maybe by you but I've seen a ton of examples this way which is why I did it this way. So when I replied I said that I tried it the other way but it didn't work. So they mark it resolved but then also marked "won't fix". And then a snarky comment at the top that some items were not addressed but under deadline pressure. Like no, it wasn't fixed because it was correct and changing it would have caused a bug, asshole.
Whew... Deep breaths.
So they finally resolve it with "don't want to go round and round about simple stuff".
Aha! You got 'em!
Screenshot that bit, and then every time they do a pedantic review thing paste it in and then mark the comment as resolved and move on.
Well boss pointed out the bug discrepancy, so he's paying attention. When I asked if it was just me being sensitive or something or of he had talked to team lead, he said no, he was aware. So, yeah...But it didn't seem to be improving.
Coming from someone in the 'boss' role, it sounds like you might be in the evidence gathering phase, probably leading to a bad review/PIP/etc for the pedantic dev. It's really frustrating how this stuff always takes way longer than it should, but a good manager will eventually get things moving here.
So your boss is aware. And how do they want you to handle it?
Well boss pointed out the bug discrepancy, so he's paying attention. When I asked if it was just me being sensitive or something or of he had talked to team lead, he said no, he was aware. So, yeah...But it didn't seem to be improving.
Coming from someone in the 'boss' role, it sounds like you might be in the evidence gathering phase, probably leading to a bad review/PIP/etc for the pedantic dev. It's really frustrating how this stuff always takes way longer than it should, but a good manager will eventually get things moving here.
Well we both started in May, so haven't been there 90 days yet. I assume they could just show them the door at any point right now. We also had another dev on 2 week vacation that had been planned for a while. So they come back next week.
For all I know boss is interviewing people. It's not like I have full access to his schedule, just busy/ooo status. 
There was also a checkin way back in our 2nd or 3rd sprint where pedantic broke some of the main functionality of the app, like this whole dialog wouldn't save anymore. Had to roll back that PR and redo it all for another week or two. Then that might be what started causing these other bugs? I don't know.
I've cleaned up for people breaking the main/prod build a few times at previous job, but thankfully have never been the one responsible (knock on wood). I just went and got latest and said hey, something is wrong, and managed to track down the issue a couple times.
This time I was working on a different part of the app so didn't get hit with it. But everyone else was stuck for half a day. That was the first warning sign for me.
I use Visual Studio Code with black and flake8 (considering switching to ruff). There are tons of extensions that implement lots of formatting and best-practices type stuff. I’ve never used MSVS for Python.
We use black and flake8, even with IntelliJ/PyCharm.
I still hate IntelliJ.
Stele - so this guy started at the same time as you and is not like your supervisor or something? I assumed from your post he was someone that had been there forever and was like a lead having to approve your stuff or the QA person that has to sign off on releases. If you guys are peers, can you just tell him thanks for the comments, it works as is and please merge?
Stele - so this guy started at the same time as you and is not like your supervisor or something? I assumed from your post he was someone that had been there forever and was like a lead having to approve your stuff or the QA person that has to sign off on releases. If you guys are peers, can you just tell him thanks for the comments, it works as is and please merge?
Right we're both senior devs. And we have a FE lead and a BE lead on a team of like 10. So I'm just trying to go by the rules as laid down by the leads, this is how we handle PRs, etc. But it feels like pedantic is using it for... I don't even know what? To show off his knowledge? To slow down my work? To try to pretend I have as many bugs as they do? 
I don't really get it. And I haven't checked every single code review to see how much they are doing this to others.
This week I felt trapped because other FE devs were all out on the same day so it was just the one person that could review. But even so... the one about 2 weeks back, another dev had already made 2 comments, (unused imports and leftover comments I forgot to clean up)... I had committed changes, and they approved. Then while waiting on QA approval, pedantic jumped it and made like 6 more comments on an already approved PR. I was floored. And that's when I started asking the leads and boss how the heck I was supposed to handle this?
And when I have responded as others have suggested, I still get the pushback... see a few posts above about how I linked directly to their approval on the exact same lines of code from a previous review and they still asked me to change it. It's wild.
Now here we are like the 4th time my work has been held up for hours at a time in 3 weeks or so, 90% for useless changes, and I'm just wondering WTF. But I put it here instead of "how's work been?" because of the specific PR/code review processes, and seeing what others had seen. And so far not a lot of "oh yeah I knew a dev like that". Mostly shock and surprise, and suggestions for the team to not let bullshit like this slow down work. So I'm not losing my mind, thanks everyone.
Does your team have a rule that all comments most be resolved before a PR is merged? If so, then from what I'm hearing you might as well start changing your task statuses to "blocked" when they're ready to merge except for comments that you've replied to.
Side note, it's always interesting to hear how differently teams work, and I'm curious about the part where somebody in your org is tracking how many bugs came from which engineer. Where I live that would be weird to do even as a joke. 
I will say that part of our SOC2 compliance is making sure we have strong processes for things being approved by multiple people before they get released to customers.
For our part, that includes enabling settings in GitHub that require code only be merged by pull request, and that pull requests pass all tests and that any review feedback be marked resolved before merging is allowed.
Well we both started in May, so haven't been there 90 days yet. I assume they could just show them the door at any point right now. We also had another dev on 2 week vacation that had been planned for a while. So they come back next week.
Oh you're in the first 90 days? Yeah... they could be gathering evidence for the 90 day 'hey we don't want you here after all' conversation. Or there could be other things going on like 'I will lose the headcount if they leave, so better something than nothing'.
Or they could just be a bad/conflict-avoidant manager, happens a lot. Sorry you have to deal with it.
Couple of updates if anyone cares. Boss said he (and lead?) were having a talk with Pedantic, AKA "Doing Too Much", master of creating bugs, a few days ago. Told me that if any more PR issues come up to let them know.
Today, a bug fix PR came in with a couple lines changed for the bug, and like 10 other changes for "refactoring" shit that didn't have anything to do with the bug. Which seems to be how Doing Too Much is creating so many bugs to start with. They clearly missed the part on KISS in college or several previous jobs. So lead REJECTED THE PR. 
Then a new one was submitted with the 3 lines needed to actually fix the bug.
Meanwhile another bug fix from a couple days ago was merged in with that... I got latest to start my new task. And now I'm getting unit test errors from Mr Doin Too Much's code over in the other part of the app. Maybe 60-70% of the time I run test, theirs fail. So we got a flaky problem going on... I let the lead know, and he said he was seeing it too.
At this point I think we'll get more done without them, and we'll certainly have less bugs. Hopefully boss is willing to take the next step before things continue to degrade.
I know I'm preaching to the choir, but maybe a good reminder for DTM Person:
Agile Methodology is the art of maximizing the amount of work NOT done. Not to say that developers are lazy; rather, by not doing work that is not strictly necessary to complete the current task, that tends to let developers move on to build the next feature sooner.
Value pace over perfection. Code is rarely truly immutable; we can always change it later if change is warranted. (note: this assumes the code is not in control of literal life-or-death scenarios)
Did the linting situation get sorted out? I still wonder if that's part of the root. If the extraneous changes he keeps putting in are fixes for issues flagged by reasonable linter settings, then even if one disapproves it's easy to understand why he wants to make them...
Pages