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

Reorient GitHub pull requests around changesets

mitchellh.com

Post reply on HN
Print view View original

91–100 of 210 posts

Re: Reorient GitHub pull requests around changesets

#91

Post by neo2006 »

duncan-donuts wrote: post #73
neo2006 wrote: post #67

Earlier quoted context omitted.

I don't see code review as an overwatch but a good communication tool and a way to think about problems collectively. Code review reduce bugs because it permit to have people think about the problem from multiple angles. About decisions, I think important design decisions need to be taken prior to the code review steps ,and also reviewed. I'm not sure what context you worked in that gave you that opinion about code r…

I don’t mind code reviews so I’m not really defending who you’re replying to. There is another way to think about this tho and I actually have found this practice to be light years more effective than code reviews. Where I work we have a practice where before you start doing a ton of work on your card/ticket you should find someone on your team, explain the problem to them, and show them how you’re going to do the wo…

I fully agree, we also do this and this is the design review I alluded to. That said, I think both have different purpose. The design review is more about the approach and the high-level implementation. The code review is about the implementation details. Also, a great aspect often overlooked around code review is that it's a great teaching tool if used right. Asking question go a long way and make both the reviewer and the reviewee learn. Code review is often looked to as a senior watching over what juniors are doing. In my experience it should be done for all levels and even juniors should review seniors PR as they can bring novel perspective and at least learn from the code by reviewing it.

Re: Reorient GitHub pull requests around changesets

#92

Post by piotrkaminski »

0x073 wrote: post #79
piotrkaminski wrote: post #29

Earlier quoted context omitted.

Here's how I like to think about it: GitHub is a generalist. They have a big platform with lots of features besides code review, so even though they also have lots of employees they won't be able to focus on code review as much as a dedicated company could. They also have a huge number of users to please so they can't afford to rock the boat too much or make the learning curve too steep. I think therefore it's pretty…

What's the reason as a company to pick GitHub? Pull request and everything around it are by far the most important part. If i must pay for GitHub and an external tool, isn't GitHub just an dumb overpriced git storage.

Most frequently, I think it's because you want a single platform to store all company code, but not all teams agree on using GitHub PRs vs a more advanced code review tool. Other potential reasons include having access to the other GitHub features: issues, actions, security stuff, the merge queue, etc. You could pull all these together from less-overpriced more-specialized alternatives, of course, but sometimes it's nice to have a single integrated platform even if you decide to replace one of its features.

Re: Reorient GitHub pull requests around changesets

#93

Post by jacobegold »

fahhem wrote: post #88
jacobegold wrote: post #85

GitHub actually stores all the data necessary to do this without changing your ref model as the article suggests — even if you force push, it never garbage collects the commits that the branch used to point to. I work on a tool that includes a UI for diffing versions of a GitHub PR, but you can totally get the same thing via messing with GitHub.com URLs.

I agree, we work on Reviewable, which uses those same blobs. However, we add tags to commits we're referencing, just in case someone at GitHub gets around to implementing garbage collection!

They must have refs on the internal git servers -- they need them for the "force pushed from X to Y" timeline events

Re: Reorient GitHub pull requests around changesets

#94

Post by tomasreimers »

jacobegold wrote: post #85

GitHub actually stores all the data necessary to do this without changing your ref model as the article suggests — even if you force push, it never garbage collects the commits that the branch used to point to. I work on a tool that includes a UI for diffing versions of a GitHub PR, but you can totally get the same thing via messing with GitHub.com URLs.

Hi! I work with the commenter - for those wondering here's documentation on that feature (pulled off with pretty much only GH data):

https://graphite.dev/docs/pull-request-versions

If people are interested, probably a thing we could do a technical blog post on.

Re: Reorient GitHub pull requests around changesets

#95

Post by fahhem »

jacobegold wrote: post #93
fahhem wrote: post #88

Earlier quoted context omitted.

I agree, we work on Reviewable, which uses those same blobs. However, we add tags to commits we're referencing, just in case someone at GitHub gets around to implementing garbage collection!

They must have refs on the internal git servers -- they need them for the "force pushed from X to Y" timeline events

You're right, though I think that changed recently? I vaguely remember only the last force push showed up, but now I see more than one so :shrug:

Re: Reorient GitHub pull requests around changesets

#96

Post by jacobegold »

Aeolun wrote: post #28

Hmm, not saying Github is perfect, but I think there’s value in providing the simplest possible experience as the default.

I was lucky enough to work at a company with a great code review tool at one of my first positions -- and I am nowhere near convinced that GH's review interface is the simplest possible.

Of course, everyone (including myself!) is probably biased to think that whatever they are used to is simplest, tbh.

Re: Reorient GitHub pull requests around changesets

#97

Post by tomasreimers »

imran-iq wrote: post #21
m3drano wrote: post #3

IIUC, he's referring to a workflow that Gerrit implements 1-to-1, isn't he?

It's the email flow that's existed before gerrit and phabricator. Both of which implemented because it makes sense. Then github came along and here we are.

This. Every time people talk about how pull requests /should/ work, I point to them that's largely how they did work...

Re: Reorient GitHub pull requests around changesets

#98

Post by epolanski »

neo2006 wrote: post #67
epolanski wrote: post #62

Jm2c, but by far the best place I've ever worked (my current client) we don't do code reviews at all unless the author wants feedback. PRs are good ways to defend your code base from bad code, and they were born in open source where you literally have no clue who the contributor is, but years of experience left me convinced that I don't want such a system where there's constant need to overview each other's work. I w…

I don't see code review as an overwatch but a good communication tool and a way to think about problems collectively. Code review reduce bugs because it permit to have people think about the problem from multiple angles. About decisions, I think important design decisions need to be taken prior to the code review steps ,and also reviewed. I'm not sure what context you worked in that gave you that opinion about code r…

I'm gonna rephrase my previous point. What you say makes sense, but only if you understand that it takes away time, money and energies and after knowing it, and the impact of all this time drag you determine it's worth it.

Might apply to huge products making millions $ every day. Sure, delivering a bug will be expensive.

Might apply when you can't trust your colleagues (not skilled, reasonable or experienced enough).

Might apply if your code is niche but mission critical (maybe some safety system on a car or a dangerous tool, or surgery equipment).

Of course I agree with all you said.

But you're writing some Java/TS web app which isn't raking much money, or has still to be launched, or your biggest focus is time to market to beat competitors and you're wasting time on code reviews the author hasn't requested?

In this scenario (my current client) I want to have a team I can trust and gets stuff done. This does not imply that CRs don't happen or design isn't discussed, but it happens when it brings value or it is needed. And I like it that way, where we can build stuff, rather discussing how to build it.

Re: Reorient GitHub pull requests around changesets

#99

Post by Pxtl »

Azure DevOps has some of this, it has "pushes". You view the history of a PR per-push, so developers can commit early and often.

Still many of the same problems occur.

Fundamentally, I think the git emperor has no clothes. Managing commits is just too tedious, but squashing them causes too much pain.

Re: Reorient GitHub pull requests around changesets

#100

Post by jacobegold »

codeapprove wrote: post #19

I agree 1000%. I’m the creator of what I believe is a better review interface for GitHub ( https://codeapprove.com ) but there are also many others: * CodeApprove (codeapprove.com) * Graphite (graphite.dev) * Reviewable (reviewable.io) * Axolo (axolo.co) * Viezly (viezly.com) * Mergeboard (mergeboard.com) * Codestream (codestream.com) * Pullpo (pullpo.io) * ReviewPad (reviewpad.com) * Planar (useplanar.com) * Visibly…

I'm keeping an eye on Pierre (https://pierre.co/) as well — not being subject to the whims of GitHub makes for some interesting ideas!
Post reply on HN