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

Git-appraise – Distributed Code Review for Git

github.com

Post reply on HN
Print view View original

121–130 of 173 posts

Re: Git-appraise – Distributed Code Review for Git

#121

Post by Corsome »

indymike wrote: post #117
cdogl wrote: post #109

Earlier quoted context omitted.

I prefer clean code that doesn’t have comments except as doc strings for various parsers on declarations and in areas that are particularly surprising. If I want to know why something was done, I expect to be able to use git log or git blame on the file or line and get find a commit message, issue tracker link or pull request reference that I can pursue.

Issue trackers, vcs and employees change. A meaningful comment stay as long as it is meaningful.

> A meaningful comment stay as long as it is meaningful.

Sometimes even longer :)

Re: Git-appraise – Distributed Code Review for Git

#122

Post by Corsome »

mike_hearn wrote: post #114
brandall10 wrote: post #97

Earlier quoted context omitted.

"it's quicker to just fix the thing you want changed than ask someone else to do it" Sure, but I feel like there's a big disadvantage here in that the person who did the bad thing isn't actively involved in fixing it. They lose out on acquiring that muscle memory. If this is a Sr. reviewing another Sr.'s PR and it's just a few minor oversights, fine. But if this is a Sr. reviewing a Jr.'s PR and they fix like 10 thin…

You can certainly pick which approach to use based on your wider goals, sure. Sometimes though changes are a matter of style, or something is easier to show rather than tell, or you realize whilst reviewing the requirements weren't quite right (on you) etc. For example, if a junior isn't familiar enough with the standard library and misses a trick, you can try to explain in words what the better way is, or you can ju…

Yeah, additionally Juniors can read the fix commits and learn from that. Thanks for describing your work flow Mike!

Re: Git-appraise – Distributed Code Review for Git

#123

Post by foundart »

d4rkp4ttern wrote: post #110
jauco wrote: post #44

I have this "one weird trick" that I think is great, but somehow no-one else wants to use: At one point I worked with a fellow dev where we did PR reviews just as commits on top of the branch. So you'd check out their feature branch, run the equivalent of 'compare working tree with ' for your editor and then look through the changes. Because you're comparing the working tree you can leave comments... by typing source…

Along the same lines: when I want to locally review someone’s PR, I want to see their changes as “dirty” relative to main, so that the changes show up nicely highlighted in PyCharm in the “gutter” (left side). This has to be such a common need, but I am surprised that I have to do convoluted things like: create a new branch, switch to it, and then: git merge --no-commit --no-ff pr-branch Am I missing an easier workfl…

Here's a script I use for this. It's designed for use with github. Run it in your copy of a repo with a PR number and it will create a 'review' branch that has the changes uncommitted. Works great with IDEs with a good diff experience.

https://github.com/whenceforth/local-code-review

Re: Git-appraise – Distributed Code Review for Git

#125

Post by darau1 »

palata wrote: post #61
jauco wrote: post #44

I have this "one weird trick" that I think is great, but somehow no-one else wants to use: At one point I worked with a fellow dev where we did PR reviews just as commits on top of the branch. So you'd check out their feature branch, run the equivalent of 'compare working tree with ' for your editor and then look through the changes. Because you're comparing the working tree you can leave comments... by typing source…

Have you ever tried the git e-mail workflow? I think you may like it, I'd be curious to have your opinion about how it compares to your workflow above :-). The idea is that you just send your patch by e-mail, and the reviewer just applies it with `git am `. So you don't even have to sync on a branch. Then the reviewer can comment directly on the patch. You answer with new versions of the patch, and get new reviews. I…

It always amazes me that these large companies insist on reinventing the wheel.

Re: Git-appraise – Distributed Code Review for Git

#126

Post by rvdginste »

jauco wrote: post #44

I have this "one weird trick" that I think is great, but somehow no-one else wants to use: At one point I worked with a fellow dev where we did PR reviews just as commits on top of the branch. So you'd check out their feature branch, run the equivalent of 'compare working tree with ' for your editor and then look through the changes. Because you're comparing the working tree you can leave comments... by typing source…

I have been thinking about this approach, because I heard other people doing exactly this.

I am using a pull request flow currently, but have some annoyances:

- I nearly always check out the branch to review (I use a separate worktree with git worktree for this) so that I can explore the code and changes using my IDE; it is annoying to have to switch back and forth between IDE and PR interface to add comments;

- Sometimes I want to do a global review of some part of the code. This is typically not supported out of the box with review systems based on PRs. Sometimes you can work around that by creating dummy branches, but it is annoying;

- Sometimes there are interesting explanations but those are typically "lost"; yes, they are still somewhere in the PR system that was used at the time, in some pull request, tied to a commit hash that is no longer there, because the feature branch was rebased before merge; seriously, once the PR is merged everything that was commented and discussed is lost in practical terms;

So, yeah, I am tempted to try this approach with leaving comments in the code for the author of the branch. I believe it would also make the code review process more efficient.

Re: Git-appraise – Distributed Code Review for Git

#127

Post by [deleted] »

palata wrote: post #61
jauco wrote: post #44

I have this "one weird trick" that I think is great, but somehow no-one else wants to use: At one point I worked with a fellow dev where we did PR reviews just as commits on top of the branch. So you'd check out their feature branch, run the equivalent of 'compare working tree with ' for your editor and then look through the changes. Because you're comparing the working tree you can leave comments... by typing source…

Have you ever tried the git e-mail workflow? I think you may like it, I'd be curious to have your opinion about how it compares to your workflow above :-). The idea is that you just send your patch by e-mail, and the reviewer just applies it with `git am `. So you don't even have to sync on a branch. Then the reviewer can comment directly on the patch. You answer with new versions of the patch, and get new reviews. I…

[deleted]

Re: Git-appraise – Distributed Code Review for Git

#128

Post by goku12 »

Corsome wrote: post #119
goku12 wrote: post #106

Earlier quoted context omitted.

You do lose the information about where to apply the patch. This isn't a big problem for rebase-friendly crowd. But people who prefer the merge strategy strictly may not like it.

AFAICS there's `--base=auto` which captures explicit parent's sha in the patch text output (there's also a git config option to do that automatically). Source: https://git-scm.com/docs/git-format-patch#_base_tree_informa...

Thank you for the information! This is the first time I'm hearing about it. Would you happen to know how this information is used? Is it just for the maintainer, or is there a tool that can use it? (I couldn't find any other reference)

Re: Git-appraise – Distributed Code Review for Git

#129

Post by Corsome »

goku12 wrote: post #128
Corsome wrote: post #119

Earlier quoted context omitted.

AFAICS there's `--base=auto` which captures explicit parent's sha in the patch text output (there's also a git config option to do that automatically). Source: https://git-scm.com/docs/git-format-patch#_base_tree_informa...

Thank you for the information! This is the first time I'm hearing about it. Would you happen to know how this information is used? Is it just for the maintainer, or is there a tool that can use it? (I couldn't find any other reference)

Last time I checked it was only for maintainers convenience but the idea of using it by some tooling was already being discussed: https://people.kernel.org/monsieuricon/all-patches-must-incl...

Re: Git-appraise – Distributed Code Review for Git

#130

Post by uxp8u61q »

dolmen wrote: post #94

That project isn't actively maintained. For example: - Go dependencies are not kept up to date - the install instruction is from a GOPATH environment not from Go module style which is now the default for 2+ years now.

Just look at the latest commits, issues and PRs.
Post reply on HN