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

More code review tools

github.com

Post reply on HN
Print view View original

51–60 of 151 posts

Re: More code review tools

#51

Post by grouseway »

morinted wrote: post #12
willchen wrote: post #6

Does anybody else feel like GitHub has released more features in the last month than the last 6 months? I'm not sure if it's just a coincidence with all the attention they've gotten on HN, but these improvements are much appreciated!

It was probably in reaction to the Dear GitHub letter. People were considering migrating away from GitHub and so they got their hands out of their pockets. We all benefit, though, GitHub becomes a better platform for us and they probably become a more successful company.

People who defended them should take note. Complaints can lead to action. Cheering for software companies is as useful as cheering for sports teams.

If you go into defence mode whenever someone complains about your favorite VCS, OS, language, platform, editor, start menu, or whatever then you're probably doing it a disservice unless you're disputing factually incorrect information. Posting work-arounds and minimizing others people's complaints isn't helpful.

Re: More code review tools

#52

Post by js2 »

nightpool wrote: post #28
js2 wrote: post #21

It would be nice if GitHub supported a Gerrit-inspired code-review process, where instead of having to choose between: 1) piling new commits onto the existing branch/PR, or 2) force-pushing and completely losing the old commits on the server You could instead push into a "magic" ref-spec and the server would retain the original commits, but also the rewritten commits, such that a PR can have multiple revisions. This…

isn't this what the "view outdated diff" button does, or am I misunderstanding?

That button only appears when you add new commits to the existing branch/PR. If you amend any commits and force push, the rewritten commits are lost forever on the GitHub side of things. You can only find them in your local ref-log at that point.

Gerrit instead retains each rewrite of the "same" commit. It does this by requiring you to insert a "Change-Id: ..." footer into each commit message (it provides a repo hook to do this) and examines each incoming commit message to know whether that commit is a revision to a commit it's already seen.

Re: More code review tools

#54

Post by WillAbides »

grouseway wrote: post #51
morinted wrote: post #12

Earlier quoted context omitted.

It was probably in reaction to the Dear GitHub letter. People were considering migrating away from GitHub and so they got their hands out of their pockets. We all benefit, though, GitHub becomes a better platform for us and they probably become a more successful company.

People who defended them should take note. Complaints can lead to action. Cheering for software companies is as useful as cheering for sports teams. If you go into defence mode whenever someone complains about your favorite VCS, OS, language, platform, editor, start menu, or whatever then you're probably doing it a disservice unless you're disputing factually incorrect information. Posting work-arounds and minimizing…

I get what your saying, but the sports analogy might be a bit off. Most sports fans are most critical of their own teams and expect to lose every game.

Re: More code review tools

#55

Post by jakub_g »

I'll plug my work as always when talking about the topic:

I wrote a small script for Chrome/Firefox that I found useful for reviewing big PRs on GitHub. It gives you possibility to expand/collapse files, mark files ok/fail in order come back to them later.

It works with mouse and keyboard (though I've noticed there are some issues with tab order after GitHub upgraded their code, and small UI glitches, I'll try to have a look at them soon)

It's a hack on top of GitHub so it needs maintenance every couple of months, but overally it does its job well IMO.

I don't have much time to hack on it anymore, but community contribs are very welcome. I wrote some potential ideas for improvements as GH issues in the repo. AMA if you're interested.

[1] https://github.com/jakub-g/gh-code-review-assistant

Re: More code review tools

#56

Post by chinhodado »

js2 wrote: post #21

It would be nice if GitHub supported a Gerrit-inspired code-review process, where instead of having to choose between: 1) piling new commits onto the existing branch/PR, or 2) force-pushing and completely losing the old commits on the server You could instead push into a "magic" ref-spec and the server would retain the original commits, but also the rewritten commits, such that a PR can have multiple revisions. This…

Why can't you do 1), but at the end of the PR, after everything is cleared and it is ready to merge, squash/modify the commits structure however you like?

Re: More code review tools

#57

Post by mwarkentin »

erichurkman wrote: post #7
danpalmer wrote: post #2

This is a great step in the right direction. I use GitHub for code review every day, and it has historically been very poorly designed for thorough reviews. These changes look great, I just hope that we get some sort of checking-off of review points, and accept/reject functionality.

I'd love a UI way to enable `?w=1` whitespace mode, especially for html/json/yaml files. I have a feeling a lot of reviewers don't know about it and spend an inordinate amount of time squinting to see what changes when it was a whitespace-only change.

On top of that.. when you're in `?w=1` view you can't leave line-by-line comments. :facepalm:

Re: More code review tools

#58

Post by ed_blackburn »

avolcano wrote: post #3
danpalmer wrote: post #2

This is a great step in the right direction. I use GitHub for code review every day, and it has historically been very poorly designed for thorough reviews. These changes look great, I just hope that we get some sort of checking-off of review points, and accept/reject functionality.

I wonder if there's a set of API hooks they could expose to allow third party applications to handle more advanced code review techniques? I would totally understand GitHub not wanting to implement a full Phabricator/Gerrit review feature set, but maybe they could make it easier to integrate other services for it.

I agree. I think there's a whole ecosystem there.

Re: More code review tools

#59

Post by guelo »

Sometimes when I submit a PR and I get a lot of feedback I get lost making sure I've addressed every comment. My #1 feature request would be being able to mark comment threads as resolved. The outdated comment feature sometimes works for this use case but mostly it doesn't.

Re: More code review tools

#60

Post by ehnto »

js2 wrote: post #21

It would be nice if GitHub supported a Gerrit-inspired code-review process, where instead of having to choose between: 1) piling new commits onto the existing branch/PR, or 2) force-pushing and completely losing the old commits on the server You could instead push into a "magic" ref-spec and the server would retain the original commits, but also the rewritten commits, such that a PR can have multiple revisions. This…

I guess this is probably a subjective topic, but I quite like they way Assembla handles it's Merge Requests.

When you make a change, all your new commits show up in a new version, and you can switch between the different versions of the MR to see the old state of the code before each new version. Where a version is created after a push of new commits to the branch.

You still keep the entire commit history (which I much prefer), but the view of that history and discussion around it is much clearer in the MR.

Github's way of doing it makes it difficult to see a concise view of the history of the MR, which can contain very valuable information outside of the commit history.

Post reply on HN