STOP Nit Picking In Code Reviews 

I love when the deadline is today and there is nitpick code reviews that delay the review and merge process by 5 more days

I love to be called a C# loser so early in the morning. Thanks for making my day better, Prime 😊

I had an engineer say to me once “if I put a nit in a review then it means I don’t really care if you fix it. It’s just an idea” and that’s how I’ve chosen to see nits from now on.

Regarding tests that don't provide any value... My friend once told me something very interesting: He wished testing frameworks could have a single switch, that flipped all the asserts to be the inverse. Then, all your tests should fail. If any still pass, they can be deleted because they're absolutely useless.

You guys discussing about validity of nitpicks and I'm standing in the corner crying because I would like to have this problem. I would like anyone to read my request before approving it. Even if I explicitly ask for thorough review because I have doubts about my changes, they are looking for max 10 second. Their thoughts (probably): "Yes, indeed this seems to be a computer code. APPROVED!"

Most of the time it's not about right or wrong, it's about consistency. If all private variables in the project start with an underscore (_), and someone make a PR with code without the underscore because they don't like it, it's gonna be 'nitpicked' about it, even if they agree with you, because of consistency. If you want to remove the underscore for your private variables, then do so in the entire project, so that it's not inconsistent. I don't like that the consistency factor hasn't been raised in the article nor in the video. It's literally one of the pillars of programming (imo).

I love in person peer reviews. Much faster and much less bad vibes about to many comments. Plus you learn so much from the other developer

I learnt so much from getting my code absolutely bashed by people more experienced than me :)) it’s literally FREE education

For me in my company I blame management for this. People are encouraged to add their two cents in a PR just to fell like they are adding some sort of value when they are not.

The best codebase I ever worked on was one where the lead developer had OCD and would point out every small issue. Nits might be annoying, but they are great for the long term.

For us nitpick comments are always prefaced with "nit: " and can be ignored by the PR submitter as they are not important to the overall goal of the implementation. Our nits are mostly just how to simplify something (i.e. make it more readable) or improving type safety, or adding comments for temporary solutions to problems and why they are needed, for example. We keep them to a maximum of 2-3 nits (depending on size of PR), they have to come with an easy copy-paste suggestion, and we never discuss nit picks, it's either applied it or not. No stream of comments.

I always find myself regretting letting something apparently unimportant go in review. It's scary how often it's come back to bite me.

I still nit pick sometimes but I found out that people usually accept it without problems if you name them "suggestions" rather than "fixes to be done". After all they are just another category of issue, if they really are. And there's another issue: if your work environment gets emotionally affected by this often it means the programmers are too sensitive about their code and criticism, and that is not a healthy either. Sensitive programmers call every "criticism" a "nit picking" to justify their poor practices as well... That's why I keep a healthy dose of nit picking and to keep things in balance.

I like the article, but I do feel like there's an implicit assumption that it's clear what's a nitpick and what isn't... Simple example is that a variable name that's totally wrong is not a nit. A variable name that could be slightly clearer or fit with some slightly arbitrary convention is a nit, but there's a pretty decent grey area in between.

As a more junior dev I love to be nit-picked. How else am I going to learn how people usually do things unless someone tells me? The more experience I gain the less I act on every single nit comment, but at first I was making every single suggested change and asking the commenter for an explanation if I didn’t understand the reason behind the suggestion.

Indeed, a matter of noise vs signal: having a zoo of naming conventions, api styles, or, in general, different ways to do same thing, is hella noisy. The trouble of couple extra code review cycles during onboarding (and maybe few "your-convention-is-wrong"-s who fail to grasp the concept of style consistency) is peanuts in comparison to stumbling over every other function for extra minute, trying to figure if it looks different because it does a different thing, or just because we ceased to bother nitpicking some time ago.

Merge now, fix when a bug is found in prod

I once got dev blocked on a massive new feature coming in because the most senior dev (notorious for nitpicking) there reviewed it and gave me a book-long list of nit-picks including jewels like "why are you using a switch statement instead of if else? Some devs may not be as familiar with swtich statements, so its probably better to stick with if else", and I had teams from other departments on my ass "where is the feature? We want it!", so I made the call and told the most senior dev "Since the comments are not concerns about something not working but just quality of life suggestions, and given that XYZ are waiting for this to go out, and given that two other people already approved it, I think we can just proceed for now, but I will make a new branch to address these concerns still... just not a reason to hold off on releasing", and then I got fired like two weeks after that after having had an endless string of 1 on 1 interviews saying I was doing great.

the reason some c# devs, including myself, like to prefix private fields with _ is not to show other pieces of code they shouldnt access those fields (because they cant), but rather to indicate to the reader of the code that a certain variable is a field of the class theyre currently reading, its really nice to be able to look at a variable name and immediately see whether its a local variable, parameter, field, etc.

When I was a new dev I had a code reviewer who would nitpick but would still approve my code. I appreciated the additional feedback but only because it didn’t block me. If I had time to make the fixes I would. But if not I would try to transfer their advice to the next PR

99% of nitpicks belong to the realm of linters and formatters. Edit: Commented before watching the full video.

12:00 Bit disappointed if Prime doesn't think bad structure/architecture is gonna screw you, because based on my experience that's absolutely the most damaging thing that can happen, and it can literally kill projects. I think one of the hallmarks of an experienced developer is that they mainly focus on the structural aspects of code during reviews, and extrapolate in time to see what will most likely become a problematic pattern in the code base, what will increase friction during work, what will cause unnecessarily high mental load due to bad design, what will cause bugs due to e.g. implicit couplings, and what will ultimately derail the architecture of the application and hurt the maintainability. Nit picking is putting focus on things that don't have such a risk or impact associated.

Single-letter variables for scopes which span a single line are totally fine.

After the self admission from the article writer, I had to double check if it was written by a principle engineer on a team I used to be on. It was the most obnoxious thing and seriously drained my soul trying to push things through dealing with nitpicks for days and weeks, and then afterwards there was usually a new major issue to address. At some point this was sabotage. Honestly, my opinion on code reviews is that style or approach choices should always go in favor of the person who wrote the code if they feel confident enough to vocalize a disagreement to the feedback given. They’re beholden to a deadline not some other, likely more senior, engineer who just feels like imparting knowledge. If you can’t demonstrate a sure or super likely problem if the code is released, then can it. Essentially: LOGAF. Also, all code reviews should address major architectural issues first before getting into bugs and nitpicks that will probably be rewritten anyways.

If there are major things to point at in the code, I don't nit pick. But if I can't find any (real) problem, I will probably nit pick. about the example with time or getTime. I personally don't care what you do, as long as you do the same everywhere in the class/module/whatever. Just being consistent is very good if you ask me, and most of the time the LSP recommends a convention for the given language, so just use that one.

I hate prefix-based development. Underscore, dollar sign, @ sign, I hate it all.

This happened during my first job as a QA. During a triage meeting going over bug reports to determine what needs to be fixed before the release goes out. A developer decided it was time to nitpick the wording of one of my repo steps in front of 10 to 15 people. The last step of the flow I used the wording "verify the crash occurs". It basically was do this action in the app and it would crash, a very obvious bug that shouldn't go out into production. He didn't like the word "verify" because it meant that the bug was supposed to be there. I changed "verify" to "observe" literally in that meeting. It was stupid and pointless, hell the guy could of just sent me a DM in slack or talked to me in person about it. I sort of wished I pulled out a thesaurus just to troll him... all of the things I would of done differently now compared to then.

STOP VIOLATING THE COMMON LANGUAGE SPECIFICATION GUIDELINES!!! It is YOUR responsibilit, Prime, to follow the coding style of the team. PERIOD! I have a feeling Prime was a terrible C# developer. Conventions are good. Coding style guidelines are good.

Function names are important. Bad names can obscure what the function actually does. Variable names are less important. Kind of depends on how long lived and wide reaching the variable is. If it's a global variable, which is accessed all over the code base, having a good name is actually somewhat important. For params in an anonymous. single-line function nobody should care.

Addendum: for brand NEW teams nitpicking can be allowed for a few months. It can often be a way to start more important discussions about general code structure. If you entirely disallow nitpicks early I think some devs will feel like they aren't allowed to voice their opinions. For example if they want early returns and avoiding deeply nested ifs - that opinion might initially voice itself as a nitpick in a single PR but then turn out to become a valuable general agreement over code styling

One often forgets one massive point here: We read code more often than we write it (10-100x) and what one puts into the code base matters! What is a nitpick an what not is pretty much a fine edge and its just not black an white: pointing out stupid types in variable names like “elementsHashMapWithIds” can be seen as nitpicking but its actually not. Naming is hard, and often finding a better variable name directly makes the code so much better to understand, its about sorting out the way too unspecific variable names from the ones where a proper name actually matters and just “elems” is misguiding and not helpful, thats why senior deva exists because you need experience and a good amount of abstraction capabilities. Thats why you should have best practices or tools to detect them, such that the reviewer does not blame but the tool blames you. Simplifying code is not nitpicking when you take the above fact seriously, if you have a clusterfuck of for loops with a cyclomatic complexity which shoots to the moon in 15 lines, and you point out that this is complected and can be made more reader friendly thats actual very good, treat your code as you would write a book, nobody likes to read pages of vomited brain dumps and then also build up an understanding for it. Logic/safety first, reader second. Formatting nitpicks should not ever be discussed, there are tools.

Wanting code to be perfect is a tricky one - because "perfect" is of course entirely subjective. My idea of perfect is not yours. And you're not wrong. And neither am I. We have just had different experience paths. For example, I often like to invert if conditions to avoid nesting. A colleague of mine doesn't. I think I'm right. He thinks he's right. Both approaches produce perfectly functional code, and the compiler simply doesn't care - it optimises it anyway. I just believe my code is easier to follow (doesn't nest as deep). He believes my approach is counter-intuitive as you're often testing for 'opposite' of what you would normally be testing for.

Literally my definition of a nit is "here's something you might not have thought of or know about, but it's not a problem for the PR", and it's always called out as being such. I make sure they know that i only expect them to make these changes if they both agree and are already going back in to make other changes anyway.

The nit that has stayed with me to this day is when an engineer commented on my every use of a named function declaration and said to use fat arrow instead because it's "ES6 style".

My preferred approach is to include all nitpicks in a first pass but use language to ensure they know its an opinion. And if they push back I'll "Nevermind" any issue that could be a preference. But, I think the act of communicating preferences between team members helps push towards a unified code base even if it doesn't immediately lead to code changes in a PR.

I got a good recommend on this from coworker back in the day, which is to actually prefix my nitpicks with "nit:" and to also outright tell a coworker when I review their first MR/PR that anything starting with nit is just a nitpick that you don't need to address in anyway for me to approve your MR/PR, that way I can get full usage of my nitpickiness and cause as little damage to the coworker as I can ^_^

Apparently I have a completely different definition of nitpick than everyone else. I use nit: for cases when the code is technically functional but actually sub optimal in some meaningful way, bad abstraction, performance, etc.

as a junior dev, Me liky when people nit picking my shitty code.

I have a colleague that is super into "Clean Code", I avoid tagging him into reviews...

Agree, in my last job as QA Lead I put a no nitpicking rule in place. If some team feels strongly about some nit there needs to be an automatic check for it configured and taken into use.

I love it when I add new features to old code that nobody has touched in years. The new linting and formatting policies from the pre-commit take affect and now the entire file is part of the code change. There's always some Super Brain that wants to use your PR to address code quality issues from three years ago.

Now, I'm about to nitpick, but I'm also going to justify my thoughts. On the time() vs getTime() thing, I feel you could construe time() as the verb, and thus think time() is going to time something and not just return the current time. Generally, I don't think getX adds much if anything, though. Maybe you could make the case that it conveys no side effects or something? I don't know. You could also argue that when you have a setX, naming getter getX makes for nice symmetry. Ultimately, I don't think it really matters much, if at all.

I don't see problems with nits. I just see that I chose a suboptimal name or something and just accept the change. I don't understand why I should be offended.

9:08 nitpick: you mean you have an X coming

I actually really like underscores for protected and private members, It used to be a C# convention so it's not about whether the language supports access modifiers.

Adding underscores to variable names is useful if you want to provide a getter for that variable and what to keep the name short (e.g. foo.bar() instead of foo.get_bar()). I also find it useful to immediately know if a variable is a member variable or a local by just looking at its name. Conventions when consistently followed can be very useful.

I am a nitpicker and I'm trying to do it less, but when it's a team consiting mostly of juniors, then they usually don't choose to do it one way, but rather they don't know they could do it another way, so it is useful for them to know. Also, if I'm the one who deals with 80% of bugtickets, because the others either have no idea what to do or I have to guide them through it and I have to debug the shit code, then at least make it closer to my kind of shit (which is not underscore or not, but definitely includes proper variable names, which everyone admits are better in the end) so I don't have to scroll up and down to find out what "res_a" and "res_b" is amongst the 3 letter variables. To be fair most of my real nitpicking is done with suggestions which can be applied in the PR directly, and would be mostly avoidable if a formatter was used (which people refuse and forget to do, because if you format then I accept even if I don't like it) My only real crime is with the random extra spaces that do nothing. I'm sorry I know I'll burn in hell but I must suggest to remove. Also, msot people have different ideas about nitpicking. Some old colleagues thought everything is nitpicking when they're in a hurry, which is kinda fair, I usually am not that picky when it has to be done fast, but if it's 5k C code with references and pointers mixed for some microcontroller that can only be run in some shit IDE which can't lint properly, I will ask to have some consistent naming. Why? Because after refusing this, the unreadable shit was tried to be debugged for months by multiple people to find multiple memory leaks which would actually have been quite visible with proper naming.

Yo thanks, I am actually such nitpicking guy. Just invented an improvement to my review process. Added the following: 🟢 Light/trivial improvement suggestions (naming, formatting, logic shortnening) 🟡 Moderate/should change (may lead to issues down the path) 🔴 Important/must change (bugs, incorrect behavior, etc.) Where the reviewed might skip on GREEN comments if they feel like it's a lot of work to refactor or do not fully agree. This will be great!

One of my PRs was blocked because another dev wanted me to change some divs to spans because it was "more semantic". Also those divs were pre-existing and not part of my changes.

I wouldn't bring that stuff in code review, but I'd put a Linter on it so it kicks back during integration. But to say things like that are just "nits" it b.s. When writing C#, you must follow the CLS. These are Microsoft issued guidelines that keep code highly readable and consistent so when the next dev comes in, it'll all be easy to understand. And yes, capitalization matters because it makes it easy to tell what's a private vs public variable or a method, a const, whatever, at a glance. Sticking to VerbNoun is good because time() could be doing anything with the time, and descriptive names ARE IMPORTANT!

There's probably some oversimplification of the issue or even a missing perspective. In projects that involve big teams of both developers and users, code standards are important - you want the code to be self explanatory on one hand and "uniform" on the other hand. The best way I've seen of enforcing a coding standard is by having an explicit "style guide" document, in some cases referring to existing documents like the ones Google published. Nitpicking beyond the style guide was at least ignored and at most frowned upon, but within the style guide it wasn't even considered nitpicking. In my personal view, code readability is about minimizing the cognitive load of the developers r/w-ing it. For instance, in one of the projects written in python I had a new developer come in and ignore the style guide of providing type hints in functions. His view was "these types are dynamic anyway, and the variable names encapsulate them" - no, type hints are important because some of us don't want to read the source code of your function to understand what's the interface to it. Also intellisense works much better with type hints.

Just wrote a whole package with my private variables with an underscore at the start... Gonna make a refactor PR right away 💀

Trailing underscore or ''m_'' prefix are especially useful in a language where one can access method and attributes without referencing to the current instance (this or self)

As a C# loser I'm really rooting for these guys to finally invent static code analysis. They are really close

My relationship with PR's drastically improved when I stopped seeing comments as critiques, and more as a conversation. But nit picks do annoy me so much 😂

“One’s ‘scrupulous critique’ is another’s ‘principles to cherish’, Let not the assiduous be misconstrued as the fierce.” - Me

As a Junior dev, I LOVE nitpicking. I don’t know the best conventions or if there is a more idiomatic solution so I always love when seniors roast my code and get as nit-picky as possible. I don’t take it personally at all. Might change when I’m a senior but that’s my take for now

I was losing my shit the first 10 minutes of this video, but the end recommending lint really saved it xD You should agree on a code standard with your team. If you dont like underscore for private variables, state your case, take a vote/let the lead make the decision and follow the standard henceforth. What an insane work environment where people can just yolo their own cryptic code styles into a project without care for conventions or code standards. I would lose my shit if i looked through code where some private variables where underscored, some werent, some getters had the get prefix, others didnt, some linq statements were using shorthand, some xyz and others full names. But Lint and autoformating is definetly the tool to use for these issues. I should be able to tell that two classes or files are from the same project/team, it shouldnt just be up to the individual fucking around by themselves xD

Unit testing got me to write better code and refactor better. Also, there were parts of our application that we could never test because no one knew how to and it always left bugs or unnecessary calls/renders so I spent a lot of time learning how to test, creating tools and templates to make it easier to test. But it came to a point where we had too much tests and our cheap pipeline was taking way too long to run. And I wasn't even going for 100% coverage! So, ever since then I've been in a ditch thinking, ok so don't test too much but what is too much and what is too little? I think this is the best rule I've heard so far. If you can't get it right the first time, write a test for it. I think that is fair enough. But also, we should do integration tests, I mean, according to the product owner's story requirements.

The absolute craziest one I've ever gotten was someone reviewing my java code and telling me that an enhanced for loop should be replaced by an integer loop because it was faster. Literally asking me to create nanoseconds of speed increase, at most, while sacrificing code readability. Extra crazy part was that this loop only ran during startup to load some variables. Extra fun fact: this guy also refused to use maps and used switch cases with 100 fields instead. Which, yes, led to 100 case blocks as well. Similar "performance" reasoning.

Probably my biggest pet peeve can't even be called "nitpicking", but just holding up PRs because of totally arbitrary personal preferences. At least (some) nitpicking can lead to the dev going "oh... yeah I see what you mean, that is slightly objectively better", but I'm talking about "hmm, instead of 'CameraPosition' can we use 'CameraLocation' because... I personally like that word" and "can we rename this package from 'tools' to 'utils' for absolutely no particular reason even though it's going to cause 9 million merge conflicts that 10 people will have to fix? No worries, if not I'll just go completely silent and passive-aggressively stall your PR for the next 3 weeks". If it's a nitpicky opinion-based change that's very simple, I might do it to avoid getting stuck in PR review limbo, but otherwise I'll ask if we can just merge the PR now and maybe change packages/naming or other nits later (preferably, done by the person who wants that change made). Sometimes it works.

how do you prefix private variables if not with underscore? Good luck with setters and getters then

My hot take without watching: If you are upset with being nitpicked, then you might be insecure with yourself as a developer. I really don't care if someone nitpicks. If most nitpicks don't matter then it doesn't matter if it's my way or theirs, so I'm willing to oblige. The same developers who don't have the humility to work out nits are often the same ones who won't take legitimate reviews without digging their heels.

As a senior dev I nit pick a lot especially reading juniors code, especially if I'm mentor of the said junior BUT I do give approve if there is only nit picking and nothing serious. There is no reason to delay it if it's urgent or has close deadline

I think it depends on the team. Used to code with a bunch of codeforce grandmasters. They would nitpick. But defaut code they would produce would by default be about nitpick free. So overall, was an incredible experience. Depends on size of team and organisation , but if all the team can get to some level so that there is no reason to nitpick anymore, also a pretty good outcome

General design patterns and code styles should be enforced wjen relevant. I agree variable names are 9/10 times just a nit, but for some things we (my place of work) use conventions to know easily what is being done in the function: things like o_myTransaction vs nsr_myTransacion, where the first is a general object transaction, and the second is a netsuite record transaction (the erp we work on)). In this case is knda meh, but if you use tx, I’ll ask you to change it, for example

In code reviews, I make a distinction between things that need to be fixed (bugs/bad design) and suggestions of minor improvements (actual nitpicks). This can be either verbal or through tools (i.e. comment vs. task). I expect and receive the same. But perhaps my team is better integrated.

About time() and getTime() -> In my experience (which is mostly in the embedded world, so it may be biased) most engineers would see them as two different things. getTime() would simply return a value that has been produced by some formerly called method or asynchronously, while time() would more likely do the thing to produce that value (be it tick/timer subtraction, or anything else).

If one can offload all of the nit-pick stuff onto the linter, it solves two problems: - immediate feedback, doesn't have to wait until code review. - I'm not the bad guy, the linter is. (Furthermore, people _mostly_ don't take offense to linter feedback, they'll usually comply.) Won't prevent all instances of nit-pick merge blocking, but it circumvents a lot of it.

My level of nitpicking depends on how open the developer making the PR is to suggestions. If the developer is less open to suggestions unless they are crucial (eg: the code induces a bug), I won't bother him with some code style issues because the only thing it will achieve is pissing them off. If the developer is more open to suggestions I'll nitpick more with them because they might genuinely be interested in improving aspects of their coding. It all depends on the person. Usually this openness highly correlates with experience, but ideally everyone should be very open no matter their experience. I've worked with junior and senior developers who were not open to change and it was terrible in both cases. Some examples of useful nitpicking for an open developer that seeks to improve himself (typically a junior) would be cleaner re-formulation of a piece of logic, better variable names, less double negatives, less unnecessary code nesting, etc... Things that do not change anything to the output, but which makes the code slightly better. Also it depends on the time it takes to make the change. A nit that requires 3 hours + of work just for slightly improving some variable names is probably a bad idea, but a nit that simplifies a piece of code and that requires 30sec to change is probably a good idea.

OMG dude, I've been here. So much back and forth

If somebody forces a full variable name in a single-line arrow function, I am going to laugh in their face. Same goes for "for" loops.

I would love to see an online survey on how people do code review comments. There could be a list of code review scenarios, each with multiple choices to choose from. The questions could include topics such as how nitpicky you like to be, what's your tone of voice in your comments ("you should do X" / "Could X work better here?"), How small or large should pull requests be, etc.

A company I worked at once had a system where one of the requirements for getting your bonus at the end of the year was finding over a certain number of issues on average per code review, nits were kind of essential there especially when the code review was for 2 lines of code There was also a requirement to keep your own average number of issues found on code you'd written below a certain number

in all honesty i do like/prefer the underscore for private methods/variables. my reasoning is basically this - i read code more than i write code. generally i think lots of developers do this since the reading part is necessary to writing part. and by having those underscores i already know that this variable is private or not, don't have to keep that info in my mind and don't have to evoke some TS hints to tell me that this is in fact a private thingy (or not)

I had a really amazing manager, when I opened a PR they would merge in by making the changes themselves to what they felt fit and I would get to learn from it without wasting anybody's time - I was a junior in this case. I honestly think if the reviewer can't fix nitpicks themselves and merge then they waste everybody's time.

I'm on board. But can imagine lots of devs being against it, arguing "The problem isn't my nitpicking. The problem is your code is hard to review because of of tiny confusing details which draws the reader's eyes to them and prevents thinking more about the big picture".

Trying out a new approach for a month is such a great concept. My team does it all the time when contemplating a new idea. Most of the time the new idea wins by default after the trial period. Quite often people are against changing something - they wanna "keep doing what we're doing".

I feel like there is room for "minor nitpick suggestions" if you have the time, but they should be clearly marked as minor suggestions and placed separately from the important stuff. Like, you can have a section with a bulleted list of "minor suggestions" at the end of your response and that's relatively harmless, but it needs to be very clear that these are not the important part. Hell, most minor nitpicks are probably no faster to describe than fix: just make a separate commit of your own that fixes them if you care so much. In a similar note, I'm a head TA for a class which is many things, but it is also a class that teaches people to write proofs and teaches them how to use LaTeX and such things. Thus, it is important for the class to comment on small stylistic stuff that I would ignore in a higher level class, including stuff like bad typesetting. But also, it dilutes the feedback. The solution the prof and I settled on is that substantive errors get annotations, which are clearly visible on the page. Minor nits go in the "other feedback" box explicitly labeled as minor comments. This provides a clear separation between the important stuff and the less important stuff and doesn't make it demoralizing to see 700 annotations everywhere on your submission on how everything is wrong.

What I find unnecessary is when somebody wants you to do things his way even though both ways are correct or his way is even incorrect. That's when he is just harming your productivity and also gives you a feeling of even when what I did was corret I will still need to change it.

I love the fact that I passively listened to this video, and yet my body language was just the same as if I had been reading clean code: slight smile, head nodding, and a violent urge to create an elbow/jaw interface for the nitpicker.

Nit-picking makes more sense when there is less ownership of code. Eg person A writes a class but it’s very likely person B will want/have to make changes to that class. Having consistency will make the code easier to follow. Especially in languages that don’t have great code-analysis/navigation support.

Naming is _INCREDIBLY_ important if you're working on a library.

Think pragmatic, not dogmatic, and your life will improve

Single letter names are perfect for small scope or loop indexes. Longer ones would be confusing.

Had a PR being held up by the CI linter being unhappy with mixed tabs/spaces in a string. And the string wasn't even code I wrote. And it has a noqa comment too. so now I always replace tabs with spaces on those files and it passed. No human would have nitpicked that tho.

One of the team leads I used to work with used to rewrite my commit messages when I open a PR. My messages were clear and described what the changes are, nothing inappropriate or wrong, guy just didn’t agree with the wording. The most bizarre experience I ever had

This is why you need a PR comment convention. It helps distinguish between blocking and non blocking comments. IMO, nitpicks, questions and comments are still important discussions but being explicit that they are non blocking is vital to make intent of the comment known. Start each comment with either: nit, praise, suggestion, issue, todo, question, thought or chore. And then come up with your own company convention

Sometimes there are devs whose English is just not good enough to come with an adequate variable name. I have to nitpick those if I don’t want some variables to make absolutely no sense

The number of nits are directly proportional to the lack of effort. I'll give WAY more nits if I know every line is another case of they don't give a crap.

Variable names are so important, though. Especially when working with non-native speakers of English. Every bad variable names causes cognitive load which makes it harder to read and understand, and therefore likely to be annoying to maintain in future

Nitpicking also called ego review

I “defer to the author” on any matters of style and preference that meet coding standards. If I mention a nit like this, I make it clear it is not a blocking comment. I wish others would do the same.

Haha, great video. Guilty! I'm a great nit picker myself and I recognize a lot what has been said in the article. Thanks for pointing this out. I think this is the reason why my colleagues always look at me the way they do. However, at some degree in my opinion there is not much wrong with nit picking, especially if you are guiding junior developers and make them aware of certain conventions. But at some point you have to let go.

I think the important thing in code style is to respect the one already adopted in the codebase, regardless of your own personal opinion.

In my previous team we had the rule, if we cannot check on it with linting tools, don't nitpick on preferences. Like e.g. variable names, you can define a minimum chars, which case you want to use, etc. Focus on how things are build are important to review, nitpicks can be reviewed by tools. And like standardjs say, avoiding bikeshedding about code style, these debates just distract from getting stuff done.

13:00 What he is describing is the conceptual difference between testing (minute) implementation details (bad), versus testing actually expected application behavior (good)

I have to disagree with a lot of the "don't nitpick X" because I work with a lot of researchers and their code is atrocious. They aren't even consistent within the code they write and will use all kinds of shorthands for stuff that is impossible to understand if you don't know the paper they've written or whatnot. They inline everything manually in huge functions with short variable names mixing case styles and spacing. A lot of this stuff would normally fit under nitpicks but in my world it isn't some random thing.

I've had an hour + conversation regarding the use of == vs === in JavaScript because of the same issue. That conversation drained the hell out of me and we were left with the all so obvious point of, it needs to be == for this specific function.

We had this rule where markup got 2 spaces and all the big boy toys got 4. Following it to the letter turned into double-tabbing inside script tags