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

121–130 of 210 posts

Re: Reorient GitHub pull requests around changesets

#121

Post by atq2119 »

facorreia wrote: post #113

It’s very weird that PR approvals remain “approved” after changes to the PR that was approved.

This can go either way, and it's ultimately a social problem, not a technical one.

I find that I fairly often notice some minor issues, like a typo in a comment. I do want the submitter to fix those, but making them go through a full review cycles is an unfortunate waste of time as I usually trust them to be adult enough to just make the fix and submit.

So I find myself wanting to say "Approve modulo these minor issues". If a project wants to allow this, it needs to either not require approvals, or allow approvals to remain even after changes to a PR.

Re: Reorient GitHub pull requests around changesets

#122

Post by Noumenon72 »

facorreia wrote: post #113

It’s very weird that PR approvals remain “approved” after changes to the PR that was approved.

Typical PR flow on my team for PRs to dev branch is reviewer approves with comments, then submitter addresses comments with new commits and merges (unless they judge the new commits are significant changes). This makes it lighter weight to offer comments since it doesn't mean another delay for a rereview. The reviewer gets emailed about any new commits that come in, so all changes do get seen.

Github makes it easy to review just the changes since your last approval, a feature which I think obviates the need for changesets as in the OP.

Re: Reorient GitHub pull requests around changesets

#123

Post by vanous »

fahhem wrote: post #82
ghthor wrote: post #44

I’m in 100% agreement. It’s difficult handling reviews of juniors in the current model, as you have to address the fir usage first and how to avoid these type of issues ITA BeFORE getting to the review at hand. Sign me up for change sets!

Then sign up for Reviewable, which supports this (we called them revisions) http://blog.reviewable.io/tracking-changes-in-a-code-review Sorry for the shameless plug!

I understand that you have to sell, but could you please do it somewhere else or take some decency and not to have your team to plug on every other comment here? Thank you!

Re: Reorient GitHub pull requests around changesets

#124

Post by Noumenon72 »

juped wrote: post #50

I love the amazing, advanced submission and review workflow you can see if you click anything with PATCH in it at https://public-inbox.org/git/ . The best part is that it's fully integrated with Git! It's distressing to me that Github spends so much money making Git worse. (There's some good parts of their whole product lineup, but the Git integration is supposed to be the centerpiece.)

All I see is normal code review comments plus diffs, but as an email thread instead of a UI that lets comments be attached to the code they're discussing. And it's hard to read because no markdown.

Re: Reorient GitHub pull requests around changesets

#125

Post by atq2119 »

juped wrote: post #50

I love the amazing, advanced submission and review workflow you can see if you click anything with PATCH in it at https://public-inbox.org/git/ . The best part is that it's fully integrated with Git! It's distressing to me that Github spends so much money making Git worse. (There's some good parts of their whole product lineup, but the Git integration is supposed to be the centerpiece.)

I agree that this is how development should work. But let's be fair: The claim that this "is fully integrated with Git" is at least misleading.

Yes, there's git format-patch, git send-email, and git am.

But what I would really like to see in that link you shared is links that go directly to commit hashes that I can git fetch locally to see a patch or patch set in context; and links between different versions of a patch set; and so on. After all, git am does sometimes fail, e.g. because you have an incompatible base revision. And being able to push with confidence the version that you had in the email is also a plus.

Re: Reorient GitHub pull requests around changesets

#126

Post by Noumenon72 »

earthboundkid wrote: post #32
jauntywundrkind wrote: post #30

Generally, it feels like a bit of a farce that source code is very well well version controlled, but nothing else is. Data isn't well managed. Isn't version controlled well. The pull request is just another type of data. We can keep improving each applications model. But some day, imo, the general project of computing needs to take data more seriously & develop general tools for managing data over time well & consist…

I’ve said for a while that the problem with Git’s data model is that the branch information is not itself versioned. I want Git but for Git.

Git reflog provides history and rollback, what else do you need?

Re: Reorient GitHub pull requests around changesets

#129

Post by zerbinxx »

XorNot wrote: post #110
zerbinxx wrote: post #105

Earlier quoted context omitted.

One practice I’ve gotten into with juniors is checking out their PR, branching it, PR-ing to their PR, and then having the discussion about what I want them to change there. This is good because it lets us isolate certain issues (and truly resolve them) rather than comments getting pulverized by lots of little commits, only to have someone else come in and say “LGTM!” and merge it with unresolved issues. Another obvi…

I like this approach too, but almost no one understands it (i.e. the idea it's possible is foreign) and UI support for it is non-existent in all the major products. It would be wonderful if we could ditch "change these lines" type comments in favour of just letting merge requests with the changes be easily surfaced.

Yeah, it really wouldn’t be that hard to implement though. I think I’ll take a stab at something like this using GitHub cli tomorrow or Monday

Re: Reorient GitHub pull requests around changesets

#130

Post by u801e »

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…

> 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 nowadays)

This is a misconception. git itself has commands (git-format-patch and git-send-email) that automate the creation of patches and sending changeset email threads to the mailing list. The only thing one needs to do is set the appropriate configuration settings in their git config (which is a one time operation like setting your name and email address).

The actual interaction on the mailing list (responding to those who review patches and changesets can be done in any email client of one's choosing (though it's helpful to use an email client that supports threading using the Message-Id, In-Reply-To and Reference headers rather than one that only handles conversation view style replies).

Post reply on HN