Hacker News Board Hacker News, browsed like a classic message board
Live data from Hacker News

There is an AI code review bubble

greptile.com

Post reply on HN
Print view View original

211–220 of 265 posts

Re: There is an AI code review bubble

#211

Post by quuxplusone »

hdjrudni wrote: post #179
Quarrelsome wrote: post #108

Earlier quoted context omitted.

it "nit" short for nitpick? I think prefixing PR comments with prefixes like that is very helpful for dealing with this problem.

Yes, but I don't know how effective it is. 99% of the time someone leaves a 'nit' the other person fixes it. So we're still dealing with most of them like regular comments. Only once or twice I've been like "nah, I like my way better" but I can only do that if they also leave an LGTM. Sometimes they do. There's one or two people that will hold your code hostage until you reply to every little nit. At that point they…

> There's one or two people that will hold your code hostage until you reply to every little nit. At that point they don't feel like nits.

If the comment must be addressed before the review is approved, then it is not a nit, it is a blocker (a "changes required"). Blockers should not be marked as nits — nor vice versa.

I agree that prefixing comments with "Nit:" (or vice versa in extreme cases "This is a big one:") is psychologically useful. Yet another reason it's useful is that it's not uncommon for perceived importance to vary over time: you start with "hmm, this could be named blah" and a week later you've convinced yourself it's a blocker — so, force yourself to recognize that it was originally phrased as a nit, and force yourself to come back and say explicitly "I've changed my mind: I think this is important." With or without the "nit/blocker" prefixing pattern, the reviewer may come off as capricious; but with the pattern, he's at least measurably capricious.

Re: There is an AI code review bubble

#212

Post by coldtea »

johntash wrote: post #162
eieio wrote: post #144

Earlier quoted context omitted.

at my last job code review was done directly in your editor (with tooling to show you diffs as well). What this meant was that instead of leaving nitpicky comments, people would just change things that were nitpicky but clear improvements. They'd only leave comments (which blocked release) for stuff that was interesting enough to discuss. This was typically a big shock for new hires who were used to the "comment for…

What if you have two people with different ideas of how to name a certain variable and they just flip the name back and forth every release? I like this review method too though, and like that some pr review tools have a 'suggest changes' and 'apply changes' button now too

>What if you have two people with different ideas of how to name a certain variable and they just flip the name back and forth every release?

You fire both or at least one of them. Problem solved.

Re: There is an AI code review bubble

#214

Post by eieio »

jcgl wrote: post #210
eieio wrote: post #144

Earlier quoted context omitted.

at my last job code review was done directly in your editor (with tooling to show you diffs as well). What this meant was that instead of leaving nitpicky comments, people would just change things that were nitpicky but clear improvements. They'd only leave comments (which blocked release) for stuff that was interesting enough to discuss. This was typically a big shock for new hires who were used to the "comment for…

That sounds great. Was that proprietary tooling? I'd be interested in some such thing.

The tool (iron) isn't open source, but there are a bunch of public talks and blogs about how it works, many of which are linked from the github repo[1].

It used to be "open source" in that some of the code was available, but afaik it wasn't ever possible to actually run it externally because of how tightly it integrated with other internal systems.

[1] https://github.com/janestreet/iron

Re: There is an AI code review bubble

#215

Post by eieio »

johntash wrote: post #162
eieio wrote: post #144

Earlier quoted context omitted.

at my last job code review was done directly in your editor (with tooling to show you diffs as well). What this meant was that instead of leaving nitpicky comments, people would just change things that were nitpicky but clear improvements. They'd only leave comments (which blocked release) for stuff that was interesting enough to discuss. This was typically a big shock for new hires who were used to the "comment for…

What if you have two people with different ideas of how to name a certain variable and they just flip the name back and forth every release? I like this review method too though, and like that some pr review tools have a 'suggest changes' and 'apply changes' button now too

Typically in this system you encode obligations - e.g. "eieio should review, or at least be aware of, all changes made to this library." I think that means you're unlikely in practice to have a problem like that, which (unless the team is not functioning well) requires two people who care deeply about the variable name and don't know that someone else is changing it.

Re: There is an AI code review bubble

#216

Post by layer8 »

Arainach wrote: post #139
daotoad wrote: post #131

Earlier quoted context omitted.

Naming comments are useful when someone catches something like: 1. you are violating a previously agreed upon standard for naming things 2. inconsistent naming, eg some places you use "catalog ID" and other places you use "item ID" (using separate words and spaces here because case is irrelevant). 3. the name you chose makes it easy to conflate two or more concepts in your system 4. the name you chose calls into ques…

If the person reading the code doesn't quickly understand what's going on from the name or finds the name confusing, the name is poor and should be changed. It is way too easy for the author to be caught up in their mental model and to be unaware of their implicit assumptions and context and choose a name that doesn't make sense. The bigger problem is people who feel ownership of shared codebases tied to their ego an…

> If the person reading the code doesn't quickly understand what's going on from the name or finds the name confusing, the name is poor and should be changed.

I used to think that way, but in many nontrivial circumstances, every conceivable name will be a mismatch for where some person is coming from, and not be self-evident for their mental model. Even the same person, over a longer time span. There is often a gap to bridge from name to meaning, and a comment isn’t the worst way to bridge it.

Re: There is an AI code review bubble

#217

Post by layer8 »

solid_fuel wrote: post #161
spooky_action wrote: post #112

Earlier quoted context omitted.

Sorry for the dumb question, is the second version actually better than the first? Because I prefer the first. But perhaps you chose this as a particularly annoying/unuseful comment

If I was going to nitpick it I would point out that `itemsCount` could easily be confused with `items.Count`, or vice versa, depending on syntax highlighting. That kind of bug can have a negative impact if one or the other is mutated while the function is running. So clearly distinguishing the local `numberOfItems` from `items.Count` _could_ be helpful. But I wouldn't ping it in a review.

That’s why it’s `itemCount` and not `itemsCount`. ;)

(Because the correct English term is “item count”, not “items count”.)

Personally, I tend to only name it “count” if it’s a variable that is used to keep a count, i.e. it is continually incremented as new items are processed.

Otherwise I tend to prefer `numItems`.

Yes, this is very close to bike-shedding. There is, however, an argument to be made for consistency in a code base.

Re: There is an AI code review bubble

#218

Post by ta988 »

johntash wrote: post #162
eieio wrote: post #144

Earlier quoted context omitted.

at my last job code review was done directly in your editor (with tooling to show you diffs as well). What this meant was that instead of leaving nitpicky comments, people would just change things that were nitpicky but clear improvements. They'd only leave comments (which blocked release) for stuff that was interesting enough to discuss. This was typically a big shock for new hires who were used to the "comment for…

What if you have two people with different ideas of how to name a certain variable and they just flip the name back and forth every release? I like this review method too though, and like that some pr review tools have a 'suggest changes' and 'apply changes' button now too

Apply rule 8 of Go

Re: There is an AI code review bubble

#219

Post by gbalduzzi »

jcgl wrote: post #210
eieio wrote: post #144

Earlier quoted context omitted.

at my last job code review was done directly in your editor (with tooling to show you diffs as well). What this meant was that instead of leaving nitpicky comments, people would just change things that were nitpicky but clear improvements. They'd only leave comments (which blocked release) for stuff that was interesting enough to discuss. This was typically a big shock for new hires who were used to the "comment for…

That sounds great. Was that proprietary tooling? I'd be interested in some such thing.

If I understood correctly, the same can be done on VS Code with the github plugins (for github PRs)

It's pretty straightforward: you checkout a PR, move around, and either make some edits (that you can commit and push to the feature branch) or add comments.

Re: There is an AI code review bubble

#220

Post by Dylan-CodeRab »

sebra wrote: post #174

I've tried Greptile and it's pretty much pure noise. I ran it for 3 PRs and then gave up. Here are three examples of things it wasted my time on in those 3 PRs: * Suggested to silence exception instead of crash and burn for "style" (the potential exception was handled earlier in code but it did not manage to catch that context). When I commented that silencing the exception could lead to uncaught bugs it replies "You…

I am a member of the CodeRabbit tech support team, would you be able to provide me the ticket number you have open with us? I'd be happy to get this escalated internally so we can get this resolved for you ASAP.
Post reply on HN