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

61–70 of 151 posts

Re: More code review tools

#61

Post by yxhuvud »

nightpool wrote: post #32
alexwebb2 wrote: post #10

Did they kill commit-level comments? I can no longer make comments on a given commit - the entry box at the bottom is gone. Looks like you have to scroll all the way back up to the top, switch to the "Conversation" tab, and then make a PR-level comment instead of a commit-level comment. I hope I'm missing something here, because as it stands now it's a big step backwards.

They removed full-commit comments on PRs, as opposed to line-level comments on either commits or diffs, or full-commit comments on non-PR commits, but I don't think anyone used those—I haven't even seen anyone use something that wasn't a line comment for a long, long time.

Not being able to comment on the commit message is non-stop retarded though.

Re: More code review tools

#62

Post by jbrooksuk »

nightpool wrote: post #36
danpalmer wrote: post #26

Earlier quoted context omitted.

Unfortunately that doesn't work for us for 2 reasons - 1. There's a data loss bug when multiple people edit the PR descriptions. I've had that as an open issue with GitHub for ~6 months, my team experiences it several times a week. 2. We use the checkboxes to assign and track reviewers, and since there's only one count of checkboxes, it would mess with our "2 of 5 complete" kind of metric for reviews. I'd like first-…

Question: why do you use checklists to assign reviewers, when github has that functionality built in?

Because GitHub only allows you to assign to one person per issue.

Re: More code review tools

#63

Post by cmsj »

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…

+1 came here to talk about Gerrit too.

Specifically, I would love to see GitHub extend its searching capabilities, and its code review, to be able to encompass more of a Gerrit style approach.

For example, let's say you want to have two core reviewers give a positive review on a PR before it lands. Right now your best hope with GitHub is tags, and that is frail and complex. In Gerrit, it's just some configuration.

Or, being able to show yourself only PRs that you haven't reviewed yet, or maybe ones that haven't had any reviews yet. In gerrit, it's some search terms. In github, it's another big complex tag operation.

Re: More code review tools

#64

Post by cmsj »

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 don't understand it :)

GitHub has become the de-facto place to host open source projects. Improving the tooling around code merging, seems like a no-brainer, and a huge potential win for the projects using it, especially the larger ones.

Re: More code review tools

#65

Post by lukevers »

Jemaclus wrote: post #18

I think the biggest thing I want is pagination for extremely large commits. Any ideas if that's gonna happen?

Me too. The feeling when you accidentally click on a very large diff and your browser freezes for a few seconds is horrible.

Re: More code review tools

#66

Post by vvanders »

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…

Yup, of all the git based CR tools I've used Gerrit is by far and away the best.

Re: More code review tools

#67

Post by joshpeek »

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…

Hey, I'm one of the devs for this new PR stuff. This flow is something we're hoping to improve as well. Force push support is pretty second class right now, any previous commits and discussion basically gets lost. It sucks.

So we already use a internal refspec for tracking the latest PR HEAD. You can actually manually fetch this under `refs/pull/123/head`. However, this is only the latest HEAD, not any previous histories.

My idea was to expand this to include tracking all historical HEAD refs. We came up with a clever/hacky idea to string together a chain of dummy commits pointing at all the historical HEADs. This fake commit chain would only be used internally, but it would ensure git's gc reachability checks don't sweep up any old PR HEADs. Its an alternative to creating a new ref for each previous HEAD or maintaining text reflogs for an entire repo network. Both have some performance issues for larger repos.

Its something we're still working on, but it still helps to vocalize your support for wanting improved force push support. :D

Re: More code review tools

#69

Post by danpalmer »

nightpool wrote: post #36
danpalmer wrote: post #26

Earlier quoted context omitted.

Unfortunately that doesn't work for us for 2 reasons - 1. There's a data loss bug when multiple people edit the PR descriptions. I've had that as an open issue with GitHub for ~6 months, my team experiences it several times a week. 2. We use the checkboxes to assign and track reviewers, and since there's only one count of checkboxes, it would mess with our "2 of 5 complete" kind of metric for reviews. I'd like first-…

Question: why do you use checklists to assign reviewers, when github has that functionality built in?

1) jbrooksuk's answer, only one assignee. 2) Assignees are the person owning the changes, not the person reviewing. 3) There is no status associated with it, you can't say "checked" vs "unchecked", and certainly not unreviewed/accepted/rejected.

Re: More code review tools

#70

Post by jasonlotito »

grinich wrote: post #24
smaili wrote: post #19

Earlier quoted context omitted.

Definitely not a coincidence. Although it certainly begs the question of why this wasn't done earlier given the sheer amount of capital they have.

They're focused on turning that capital into recurring revenue! i.e. building out the GitHub Enterprise business This is pretty common among companies that make the consumer->enterprise transition. Also see Dropbox, Slack, etc.

> building out the GitHub Enterprise business

None of what they just released impacts the GH Enterprise side of things. Maybe it will long down the line, but suffice it to say, none of this sells or keeps customers of GH Enterprise. The reality is, we've questioned our use of GH, and while we are still paying for GHE, it's more or less because it's just not expensive enough at this point to justify switching to something else. We'd have to update some tooling, and migrating would require time.

Basically, GHE is a glorified code repository viewer, and even then, it does a substandard job of that. Seriously, the ability to browse and search painful.

So yeah, if they are focused on GHE, these features don't suggest that.

Post reply on HN