5 ms·

In between those trivial mistakes is a lot of functional code I didn't have to write myself. Very often, that's code that I personally no longer have to write

In between those trivial mistakes is a lot of functional code I didn't have to write myself. Very often, that's code that I personally no longer have to write (if I'm reviewing it, there's a good chance it would've ended up on my place if they didn't take care of it for me), or that I get to leverage in my own code. That's stuff I can thank them for. Phrasing is important. "Here's 3 things you fucked up, fix it" is negative. "Here's 3 minor nitpicks. Otherwise, LGTM, thanks for tackling this! :thumsup:" is overall positive. Both could be describing the same code review. "While you're in there, could you add unit tests for X and Y?" is asking for a favor you can then thank them for doing, "X is missing unit tests, why?" is demanding answers for a mistake. You can still help set the tone on the receiving end of code reviews as well. "Good catch!" when people spot significant bugs in my code, reacting with thumbs up emojis when people voice their concerns. Elicit feedback, suggestions for improvements - instead of "what is everything this code does wrong", it's "okay, here's a first draft - how can we make it better?". The goal is not to deliver perfect code. That's impossible - we're human. Trying to do so anyways is counterproductive - static analysis, and a second set of fresh set of eyes will find some bugs faster. Seperating onself from one's work like that can be difficult - but between setting an example yourself, and showing geniue appreciation for the person's work even as you're finding issues in the code can help.

ambicapter 8y ago
I hate when people says LGTM but still have nits to pick. Either its ready to ship or its not.
- nitrogen 8y ago
  How about "looks good so far"?
- MaulingMonkey 8y ago
  It's ready to ship. Because nothing ever ships without issues. When talking about end products, we're often - if not usually - talking straight up known bugs, not mere nitpicks. Hell, I'm willing to OK known bugs in changelists under some circumstances - like being less buggy than what's currently checked in! Serious bugs can be a good reason to hold off on something. But nitpicks - say, a minor quibble about function naming is a subjective personal preference that should never devolve into a lengthy debate holding up an important improvement that otherwise checkin. We both have bigger, more important dragons we could use our time to slay. It's worth discussing briefly - maybe we can achieve consensus and find a better option - but if you want to check in your code as is, right now, for pretty much any reason whatsoever? Go for it. We can always make a followup changelist. Hell, please do - those are easier to diff! Even if we're working in "commit straight to shared master branch in perforce" style. If you're using git, but are worried other people about to start refactoring and want to merge into the main work branch ASAP? Go for it. Naming debates can wait. Typos can wait. There's always the next changelist. Just have a general dread of having lots of files checked out? Go for it. Make your commit. The issues are nitpicks, not blockers. I've seen plenty of good work - changelists that are objectively improvements - die in analysis paralysis and never get checked in because of quibbles over less important details. It sucks. Minor naming quibbles should not be a cause of this. Significant design problems - maybe - but not minor easily fixed after the fact naming quibbles. "LGTM otherwise!" is also a good finisher to signal "Okay, I've gone through everything, I think I won't find anything else." In more relaxed review environments, it's a signal that I don't feel the need to re-review your change - maybe with minor fixes for outright bugs - before you commit. It is carte blanche to move forwards.
lugg 8y ago
Mostly agree. Might want to rethink the nitpick thing. > The goal is not to deliver perfect code. That's impossible - we're human. Trying to do so anyways is counterproductive. If it's not important why are you saying anything? If it's code style, well if it's code style I'm going to ignore you. And if pressed I'm going to tell you to fix the linters because I ain't got time for this shit.
- MaulingMonkey 8y ago
  > If it's not important why are you saying anything? If I can take the 5 seconds to type up something I've seen, you can take the 5 seconds to fix it. We're not trying to ship perfect code, but we are trying to ship good code - so we might as well take advantage of any easy wins like fixing spotted issues. Sometimes my "nitpicks" turn out to fix/catch serious bugs that I didn't realize were bugs. Sometimes they're unimportant and ignored. I don't always know which way it's turning out ahead of time. Sometimes they're important, ignored, and then things break when it's checked in. Whoops. Easily fixed though. > If it's code style, well if it's code style I'm going to ignore you. And if pressed I'm going to tell you to fix the linters because I ain't got time for this shit. If it's exact whitespace rules, sure, I probably don't care either. I might mention it in passing so you can fix your autoformatter rules so the people who do care get off your back. If you're making me spend my time adding CI steps/rules, and build server time executing them, because you can't be bothered to match basic function casing rules? I'm going to be annoyed. Especially for more important code style issues that are hard to teach the linter about, like "dnt abbrv evrythg its unrdble", and "don't name your indicies ijIl in the same scope."
  - lugg 8y ago
    Code formatting, code style, and linting is a solved problem. If you're relying on individual developers to setup their Dev Env to do this you're doing it wrong. You need to have it automated with commit hooks. Anything else is a solution that results in aggregating nitpick reviews that ignore glaring structural problems.
    - 8y ago

ambicapter 8y ago

I hate when people says LGTM but still have nits to pick. Either its ready to ship or its not.

nitrogen 8y ago
How about "looks good so far"?
MaulingMonkey 8y ago
It's ready to ship. Because nothing ever ships without issues. When talking about end products, we're often - if not usually - talking straight up known bugs, not mere nitpicks. Hell, I'm willing to OK known bugs in changelists under some circumstances - like being less buggy than what's currently checked in! Serious bugs can be a good reason to hold off on something. But nitpicks - say, a minor quibble about function naming is a subjective personal preference that should never devolve into a lengthy debate holding up an important improvement that otherwise checkin. We both have bigger, more important dragons we could use our time to slay. It's worth discussing briefly - maybe we can achieve consensus and find a better option - but if you want to check in your code as is, right now, for pretty much any reason whatsoever? Go for it. We can always make a followup changelist. Hell, please do - those are easier to diff! Even if we're working in "commit straight to shared master branch in perforce" style. If you're using git, but are worried other people about to start refactoring and want to merge into the main work branch ASAP? Go for it. Naming debates can wait. Typos can wait. There's always the next changelist. Just have a general dread of having lots of files checked out? Go for it. Make your commit. The issues are nitpicks, not blockers. I've seen plenty of good work - changelists that are objectively improvements - die in analysis paralysis and never get checked in because of quibbles over less important details. It sucks. Minor naming quibbles should not be a cause of this. Significant design problems - maybe - but not minor easily fixed after the fact naming quibbles. "LGTM otherwise!" is also a good finisher to signal "Okay, I've gone through everything, I think I won't find anything else." In more relaxed review environments, it's a signal that I don't feel the need to re-review your change - maybe with minor fixes for outright bugs - before you commit. It is carte blanche to move forwards.

lugg 8y ago

Mostly agree. Might want to rethink the nitpick thing. > The goal is not to deliver perfect code. That's impossible - we're human. Trying to do so anyways is counterproductive. If it's not important why are you saying anything? If it's code style, well if it's code style I'm going to ignore you. And if pressed I'm going to tell you to fix the linters because I ain't got time for this shit.

MaulingMonkey 8y ago
> If it's not important why are you saying anything? If I can take the 5 seconds to type up something I've seen, you can take the 5 seconds to fix it. We're not trying to ship perfect code, but we are trying to ship good code - so we might as well take advantage of any easy wins like fixing spotted issues. Sometimes my "nitpicks" turn out to fix/catch serious bugs that I didn't realize were bugs. Sometimes they're unimportant and ignored. I don't always know which way it's turning out ahead of time. Sometimes they're important, ignored, and then things break when it's checked in. Whoops. Easily fixed though. > If it's code style, well if it's code style I'm going to ignore you. And if pressed I'm going to tell you to fix the linters because I ain't got time for this shit. If it's exact whitespace rules, sure, I probably don't care either. I might mention it in passing so you can fix your autoformatter rules so the people who do care get off your back. If you're making me spend my time adding CI steps/rules, and build server time executing them, because you can't be bothered to match basic function casing rules? I'm going to be annoyed. Especially for more important code style issues that are hard to teach the linter about, like "dnt abbrv evrythg its unrdble", and "don't name your indicies ijIl in the same scope."
- lugg 8y ago
  Code formatting, code style, and linting is a solved problem. If you're relying on individual developers to setup their Dev Env to do this you're doing it wrong. You need to have it automated with commit hooks. Anything else is a solution that results in aggregating nitpick reviews that ignore glaring structural problems.
  - 8y ago