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

91–100 of 151 posts

Re: More code review tools

#91

Post by sdesol »

jasonlotito wrote: post #70
grinich wrote: post #24

Earlier quoted context omitted.

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 somet…

> the ability to browse and search painful

Hey just an FYI, we (my company, not GitHub) are working on improving the browsing/searching situation for GitHub and GitHub Enterprise. You can see a list of the improvements at:

http://gitsense.github.io/github+gitsense.html

We've only really started advertising in the last 3 months, but what has been quite clear is, people really overestimate what GitHub and their API's are capable of. This is not a criticism against GitHub, but more of a high praise for how well they have cultivated the GitHub brand.

People really think GitHub not implementing commits search, or not being able to search forked repositories (which is pretty bad from an Enterprise point of view), is because it's not sexy, which is why they haven't implemented it yet. And when they see what we are capable of doing with GitHub, they just assume we are using GitHub's API, when in reality, the only time we use GitHub's API is to get somebody's avatar.

We obviously can't complain though, as it provides us with a business opportunity.

Re: More code review tools

#92

Post by gfunk911 »

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 didn't know about this and I love it.

Re: More code review tools

#93

Post by gmartres »

joshpeek wrote: post #67
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 histo…

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

YES! I wish for that feature everyday. Gerrit does it exactly right.

Related to this, I really, really hope that you will consider displaying commits in a PR in the correct order, suppose that I have three commits on top of master:

  a->b->c->master
If I git rebase -i master and reorder my commits so that they look like this:

  c->a->b->master
Then the commit list in the PR will still be displayed in chronological order instead of DAG order, this is very confusing! The only workaround I've found is to amend every commit in my PR so that the timestamp order match the DAG order, this isn't very fun to do.

By the way, the post explicitly says:

> Some teams choose to use a commit-by-commit workflow where each commit is treated as the unit of change and is isolated for review.

I'm glad that you're acknowledging that. But then why do comments on commits still get hidden when they correspond to an outdated diff? I would love to be able to comment on individual commits, but if half my comments are hidden because they correspond to lines that don't match the current diff, then I'm not going to do it for fear of my comments being missed. You should not hide important information that I'm trying to communicate! Here's a better way to do it: Add a "Done" button like Rietveld/Gerrit for every comment (this is a very useful feature on its own) and hide comments _if and only if_ the "Done" button has been clicked.

Anyway, I'm glad that people are working on this stuff and that they're listening to the community, thank you!

Re: More code review tools

#94

Post by slackpad »

faitswulff wrote: post #9
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.

It's interesting that you mention checking off of review points. I had begun to do that in an informal fashion by updating my pull request with notes in the form of a checklist: - [x] Refactor _ - [ ] Rename variable x - [ ] ... Now that you mention it, it would be very nice to have something like Google Docs's ability to mark comments as resolved.

This is always a deal breaker for thorough code reviews - it's always super hard to make sure that everything got resolved in a later commit. The only tool I've seen handle this well is SmartBear's Code Collaborator - you mark certain comments as defects, it does a decent job keeping them aligned with their context as revisions happen afterwards, and then you can close them out before marking the review LGTM. Unfortunately, Code Collaborator is horrible at basically everything else. It would be nice to have an easy-to-use todo list that's easy to close out and not lose context.

Re: More code review tools

#95

Post by bradfitz »

joshpeek wrote: post #67
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 histo…

Btw, I've been gluing Github PRs and Gerrit together with https://github.com/LetsUseGerrit

Example in action: https://github.com/grpc/grpc-go/pull/570

Re: More code review tools

#96

Post by chrisweekly »

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…

Home teams have an advantage in most sports, and crowd noise is a factor. More relevant to your point though: encouragement and moral support are important to many OSS projects, where burnout is a particular risk. Maybe not applicable to github per se? /random thoughts

Re: More code review tools

#97

Post by baudehlo »

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!

My guess is the switch from zero management to a more top down approach made the top realize nothing was moving forwards. GitHub had been stagnating for a while until then. But I could easily be wrong.

Re: More code review tools

#98

Post by sytse »

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…

We're thinking about adding this feature in GitLab in https://gitlab.com/gitlab-org/gitlab-ce/issues/13524

Re: More code review tools

#99

Post by Perceptes »

numbsafari wrote: post #4

I'd love it if there was a way for me to queue my comments before submitting. I often keep my comments in a separate textedit/nv window and then go back to put them in since I want to keep track of questions that arise as I'm reading, but I don't want to pepper someone with comments that would be resolved 200 lines later in the diff.

This is what I was expecting this announcement to be. It's the most often requested change for GitHub Issues that I've seen. I hope they follow up with this soon.

Re: More code review tools

#100

Post by eridius »

joshpeek wrote: post #67
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 histo…

Count me in on those who really want better force push support. Losing all the conversation on old commits sucks, but what also sucks is not being able to see a diff between the old and the new versions. If GitHub can track the history of HEAD for the PR and show diffs between them that would be excellent.

The dummy commit approach you outlined sounds like a good idea. It might be nice to expose that externally similar to how `refs/pull/123/head` and `refs/pull/123/merge` work, so that way we can handle force-pushes to PRs better in our own tooling. I'd suggest something like `refs/pull/123/history`.

Post reply on HN