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

111–120 of 210 posts

Re: Reorient GitHub pull requests around changesets

#111

Post by neo2006 »

epolanski wrote: post #98
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'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).…

If we want to include the cost of a code review into the equation we should also include the cost of fixing a big that made it to production which is in most of the cases higher then code review. Skill is not a factor here, I work and worked with some of the best engineers in the world and everyone write buggy code sometimes. Software engineering is a complex practice. If you are writing some prototyping code or some code that will never make it to production or in a very early stage of a startup, sure I can understand your point.

Re: Reorient GitHub pull requests around changesets

#112

Post by akoboldfrying »

It's surprising to me that GitHub currently doesn't attach review comments to specific commits, but only to timestamps, and I agree that it would be great to improve that.

But I don't understand how a contributor can feel pressure to address all of the reviewers' comments in a single commit: They can commit as many times as they want, and only push when they feel it's ready. And in the case where they may want to offer multiple different solutions for a reviewer to choose from, I like a small adaptation of a suggestion I saw by another commenter here, which is to make a PR for each option off their existing PR's branch: When one of those option branches is accepted and merged, GitHub will fast-forward the original PR's branch to include those commits, which is exactly what you would want and expect.

Re: Reorient GitHub pull requests around changesets

#114

Post by RHSeeger »

epolanski wrote: post #102
RHSeeger wrote: post #83

Earlier quoted context omitted.

I trust and respect the people I work with. I also prefer to have code reviews, even (especially) of my own code. Code reviews can point out code that - Doesn't consider an edge case (NPEs being the most common) - Doesn't match up well with the "standard" way we do things (and, all things being equal, using the same approach in every place improves maintainability) - Has a simpler approach - Doesn't have a comment wh…

None of the things you said matters if your issue is time to market or you have little business. Who cares about an edge case no user's gonna see? Who cares about an edge case for a non business critical feature? Who cares if there was a better way to implement something if it's not a priority? Our work it's not meant to give us devs intellectual challenges but solve people's problems. All the things you list are rea…

My goal is to build things that work, and can continue to be worked on and improved over time; where development doesn't grind to a halt in 6 months because nobody cares about quality.

Edge cases matter sometimes matter and sometimes do not. They particularly matter in cases where, if the happen, the leave the system in a bad state (corrupt data, etc). Having a second pair of eyes take a look at the code and think "where can this go wrong" doesn't cost much, but can save a lot of pain later.

Doing things in a simpler way and/or a way that matches the way we generally do things makes it easier to maintain. And it's fairly common for code maintenance and/or further development on the same code, to be more common than writing brand new functionality (code that doesn't touch existing code). This is directly related to the "grind to a halt" I mentioned earlier.

Literally nothing I mentioned can be solved by hiring standards or pay. Once, because hiring people new to software development is a thing. Two, because everyone... EVERYONE makes mistakes. And EVERYONE can do better with the help of their peers.

I prefer that every PR get reviewed by someone, with the (uncommon) exception of those where the developer says it isn't worth a review. It generally takes very little time, and it adds to delivered code quality.

Re: Reorient GitHub pull requests around changesets

#116

Post by quacker »

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…

Agree. The better your team members, the less need there is for code review.

However, the approach of few/no code reviews doesn’t work for large open source projects that accept external contributions (the author of the post is mitchellh who is a founder of HashiCorp, which has/had several large open source projects).

Re: Reorient GitHub pull requests around changesets

#117

Post by ezekg »

akoboldfrying wrote: post #112

It's surprising to me that GitHub currently doesn't attach review comments to specific commits, but only to timestamps, and I agree that it would be great to improve that. But I don't understand how a contributor can feel pressure to address all of the reviewers' comments in a single commit: They can commit as many times as they want, and only push when they feel it's ready. And in the case where they may want to off…

I think this is what https://graphite.dev is trying to do.

Re: Reorient GitHub pull requests around changesets

#118

Post by parentheses »

Changesets seem like a UX nightmare. While I understand the motivation, the complexity of version control today is mind boggling.

We have a working copy, index, commits, branches, remotes, pull requests - all of these come into play when proposing even the simplest change to an open source repo today. The idea that adding yet another concept to the pile will make things better is something I can't agree with.

Will it enable more capabilities? Yes. Adding features generally does that.

Aside from the "it's already complex enough" argument, there's also the fact that 90% of changes I've seen in my daily use of git don't require this feature. This means the feature will be misunderstood, misused and often not used when actually needed.

Re: Reorient GitHub pull requests around changesets

#119

Post by __david__ »

noirscape wrote: post #37

I'm pretty sure this approach is somewhat similar to the mailing list git approach, where patches usually get submitted once ready and then changed depending on feedback as a wholly new submitted patch (as part of a broader conversation). It'd be a useful thing to import without having to bring in the whole charade of using email and mailing lists (which most mail clients tend to be very unfavorable of in general now…

> UX would probably be more difficult though. The current workflow of "submit branch, make PR, do changes on same branch, merge latest version through web interface" is a big part of the ease of the Github UX.

That was my first thought, too. Though perhaps a simple solution could be having GitHub, on a push, check for a new branch that matches an existing branch from a PR plus “-v2” (or “-v3”, etc.) and automatically consider that a new changeset in the PR.

Or perhaps even easier is once you’ve “released” your changeset in the GitHub ui, any pushes to the branch implicitly duplicate it and put it into a new v2 branch instead. That would be a decent ui from the pushers point of view, though there’s an and asymmetry between what you push and what ends up in the repo that I’m not sure I like.

Post reply on HN