Skip to content

Instantly share code, notes, and snippets.

Embed
What would you like to do?
Pull request feature requests

Hello Github,

Pull requests are not serving me well on large reviews. Here are some problems and suggested enhancements that would make me happy.

  • Mark comment as "resolved" (see google docs for an example). As code changes, comments get lost or made redundant, and it is not clear (to either the author or reviewers) which are still relevant and which have been addressed. Resolving a comment doesn't need to make it dissappear, but I need to be able to see "which comments have not been addressed" somehow. This feature would be the most useful to me.
  • Reply to a comment. This can be done in effect on line comments (assuming there is only one), but cannot on PR comments. Particularly with many threads, the lack of this makes a review hard to follow.
  • Explicit "approve/block" life cycle. The block is actually more important to me ... often on a PR that has been incrementally improved I want to "block" final merge of it until I get a chance to rebase/squash and generally pretty it up. The block can be overridable, I just want a warning when you try to merge that "this will make XXX really sad". I had a heap of disgusting WIP commits merged to repo the other day by someone else that I will forever be embarrased by.
  • Incremental diffs. What changed since the last time I reviewed this? A quick way to change the base of the "files changed" diff would be great. Bonus points if I can force push my branch and you track the different shas and I can compare them. I do double-PRs to work around this which is not really cool.

Example large pull requests:

I'm happy to provide more detail or talk with someone on the phone if it would assist.

Thanks, Xavier

@JonRowe

This comment has been minimized.

Copy link

commented Feb 2, 2014

A way to squash PR's down when merging would be a fantastic addition to the workflow.

@sferik

This comment has been minimized.

Copy link

commented Feb 2, 2014

I replied on Twitter but wanted to leave my comments here in an expanded form, addressing each of your proposed remedies point by point:

Mark comment as "resolved"…

Line comments are already automatically hidden as new commits are pushed that change those lines. For me, this works reasonably well as a “resolved” state. I’m interested in hearing more about cases where this doesn’t work.

Reply to a comment…

As you noted, this is already solved for line comments. PR comments should be about the overall merit of the pull request (not specific code), which—in theory—should be a single-threaded conversation. If there are multiple threads, this can be solved by quoting and replying, as I am doing here. Again, curious to hear about cases where this doesn’t work.

Explicit "approve/block" life cycle…

I’ve seen this solved mostly by tagging pull request titles with [WIP] (for “Work in Progress”). When the pull request is ready, the tag is removed. This solution may be crude but has worked for me. Again, curious to see examples where this hasn’t worked.

Incremental diffs. What changed since the last time I reviewed this? A quick way to change the base of the "files changed" diff would be great.

I agree with this and would propose the following solution: the ability to easily step through each commit.

Think of a pull request as a book.

The first thing you see when you look at a book is the front cover, which displays the title and author. A pull request also has a title and an author and—just like a book—these are the first two pieces of information you should see when you look at a pull request. The current version of GitHub does a pretty good job of this.

If the title or author captures your interest, what do you do next? You pick up the book and flip it over to the back cover, which typically displays a summary and some biographical information about the author. Just like a book, a pull request has a summary, and GitHub does a good job of displaying this immediately after the title and author. So far, so good. But biographical information is missing. Has this person submitted other pull requests to this repo in the past? If so, were they merged? Are they a collaborator on the repo or a first-time open-source contributor? This information changes the way a pull request is considered and would be useful to display in this context.

After you’ve read the front and back covers, you might open the book to the table of contents to skim the chapter headings. If you think of each commit message as a chapter heading, the “Commits” tab currently makes a pretty good ToC. The commits look interesting, so you decide to start reading…

This is where GitHub’s current pull request flow falls apart. To read through the pull request you either have to open up each commit in a new tab or click on the “Files Changed” tab, which is analogous to reading the book in random order.

After reviewing the table of contents (i.e. the “Commits” tab), you should be able to easily open the book to the first chapter and begin reading. If a commit is a chapter, each diff is like a page. You should be able to leave commits on each page/diff, or general comments about the chapter/commit. At the end of each commit, you should have the option to move on to the next chapter or cherry-pick just this commit into the requested branch. On the last page of the book is a big, green merge button (assuming it merges cleanly, tests pass, etc.)

Overall, this would encourage commits at the right level of granularity and pull requests that tell a story, which is what the best ones typically do.

I’d also love to see a GitHub UI to squash commits and interactively rebase (simple conflicts could be resolved in the Ace editor). This would be revolutionary.

@holman

This comment has been minimized.

Copy link

commented Feb 2, 2014

Loving all of this- thanks for taking the time to write up your thoughts. (And we are definitely listening!)

@xaviershay

This comment has been minimized.

Copy link
Owner Author

commented Feb 2, 2014

Line comments are already automatically hidden as new commits are pushed that change those lines. For me, this works reasonably well as a “resolved” state.

This works 80% of the time, which isn't good enough for large reviews. Look at the PRs I linked and you will see both resolved comments showing, and unresolved comments missing.

approve/block, I’m interested in hearing more about cases where this doesn’t work.

rspec/rspec-mocks#543 was merged prematurely. Maybe your hack would have worked. But still, it's a hack. I like explicit approve in bitbucket, which you can almost emulate this with.

If there are multiple threads, this can be solved by quoting and replying, as I am doing here.

This works ok I guess, though it's definitely more work to cherry pick from different comments to form a single reply. Low priority, especially if they can be marked as resolved. It's really hard to follow through a specific thread without reading everything though, which makes reviewing existing conversations hard (say if I'm focussing on a specific issue to resolve it) - you basically have to read the whole thing every time.

FWIW bitbucket has reply to comment, if you wanted to play with it.

[showing bios]

This isn't relevant to me: I already recognise regular contributors, and irregular contributors I make every effort to evaluate on the merit of the code.

@JonRowe

This comment has been minimized.

Copy link

commented Feb 2, 2014

Line comments are already automatically hidden as new commits are pushed that change those lines. For me, this works reasonably well as a “resolved” state. I’m interested in hearing more about cases where this doesn’t work.

It only checks the nearest 5 lines, which for a large diff could mean the change has happened and been resolved, but the comment is still present (it also shows the old diff in the conversation view).

But @xShay is referring to when a comment has been made, discussed, and the resolution is "to do nothing", the comments still clutter the ongoing diff, so a way to force them to be "outdated" would be nice.

@myronmarston

This comment has been minimized.

Copy link

commented Feb 3, 2014

This is all good stuff. One additional thing I've really wanted (both for open and closed source projects) is a way to see what the merge conflicts are when a PR can't be cleanly merged. Bonus points if we can resolve them on the github UI. Bonus bonus points if we can rebase a PR from the github UI :).

@rsanheim

This comment has been minimized.

Copy link

commented Feb 10, 2014

/subscribe

@rsanheim

This comment has been minimized.

Copy link

commented Feb 10, 2014

Wait, that doesn't work in gist. Argh.

@dshirley

This comment has been minimized.

Copy link

commented May 19, 2014

+1 on resolving comments. Review Board offered this, and it worked great.

@dshirley

This comment has been minimized.

Copy link

commented May 19, 2014

Also +1 on being able to reply to pull-request-scoped comments

@gleseur

This comment has been minimized.

Copy link

commented May 22, 2014

+1 as marking comments as resolved

@faloi

This comment has been minimized.

Copy link

commented Jun 6, 2014

+1 on being able to mark comments as resolved.

More use cases for that:

  • when a developer doesn't understand a piece of code and needs explanation. After it is explained, the answer should be marked as resolved
  • when a discussion ends up as a new story/issue, because the current code is considered to be "good enough" for now
@aik099

This comment has been minimized.

Copy link

commented Jul 20, 2014

Line comments are already automatically hidden as new commits are pushed that change those lines. For me, this works reasonably well as a “resolved” state.

This doesn't work for me, since fact of code change doesn't mean that new code really does what was requested in comment on that line.

@aik099

This comment has been minimized.

Copy link

commented Jul 20, 2014

👍 as marking comments as resolved

@aik099

This comment has been minimized.

Copy link

commented Jul 20, 2014

Also what can be useful so far is ability to expand/collapse all outdated diff to at least be able to use Ctrl+F/Command+F on them.

@ivokund

This comment has been minimized.

Copy link

commented Aug 15, 2014

+1 to marking comments as resolved, this is something I am missing the most when doing code reviews

@deckar01

This comment has been minimized.

Copy link

commented Aug 19, 2014

👍

I just addressed a diff comment, but the fix didn't modify the commented line.
stellar/stellar-client#673 (comment)

Some comments reference visual issues and it would waste time tracking down the correct line to comment on just so the diff comment would get outdated automatically.
stellar/stellar-client#628 (comment)

Maybe add an eye or fold icon to hide the comment, then collapse the diff/comment with a "Show hidden diff" link?
screen shot 2014-08-18 at 7 09 33 pm

@dbyron0

This comment has been minimized.

Copy link

commented Oct 8, 2014

Not sure if this is too late, or piling on, but it seems related. I would love if the links in emails containing PR comments worked forever...even after the diff becomes outdated. Potentially the same issue, but I'd love to be able to send links to conversations about outdated diffs.

@cancan101

This comment has been minimized.

Copy link

commented Oct 23, 2014

An additional feature that I believe is not possible currently is the ability to write line comments on lines that are not involved in the commit itself (ie lines not part of the diff or the context around the diff). I would like the the ability to add line comments to arbitrary lines. LMK if this is already possible as is.

@cancan101

This comment has been minimized.

Copy link

commented Oct 23, 2014

I would like to 👍 that the automatic hiding of line comments is not ideal as is. I have had cases where I rename files / move code around and the comments are hidden without being addressed. Not only that but I then have to manually expand each comment to read over them and think about whether they have in fact been resolved.

@xmo-odoo

This comment has been minimized.

Copy link

commented Nov 6, 2014

Definitely in support of #1, the automatic hiding fails to hide things if the issue was fixed but not in the immediate vicinity of the line comment (very common for prose documents), and when discussion of the comment led to the original line(s) remaining. The latter issue is the most egregious as discussions can be rather long and involved until it resolves to a decision, and that seriously clutters issues. This is doubly problematic as it becomes hard to see if a line comment's thread is still active.

@danielohrlund

This comment has been minimized.

Copy link

commented Nov 18, 2014

+1 on resolving and replying to comments!

@jamescooke

This comment has been minimized.

Copy link

commented Feb 26, 2015

👍 for resolving comments

@berislavlopac

This comment has been minimized.

Copy link

commented Mar 13, 2015

Personally, I would just love to have a collapsed view of changed files, with a full diff visible only when one clicks on the file name in the list. With large PRs, especially with many removed files which don't need much reviewing, it can get pretty tedious finding the files that have relevant changes to review.

In general, it would be pretty slick to have more of Crucible features in GitHub PRs.

@hwangmoretime

This comment has been minimized.

Copy link

commented Jun 5, 2015

+1 as marking comments as resolved

@rzendacott

This comment has been minimized.

Copy link

commented Jun 25, 2015

+1 for marking comments as resolved. This would be incredibly useful. It's the only major feature missing from GitHub as a code review platform.

@thenonsequitur

This comment has been minimized.

Copy link

commented Jul 13, 2015

+1 For being able to mark comments as resolved. This is the biggest pain point for me, and affects nearly every PR. Frequently comments are resolved with "no change needed" or a code change elsewhere, and then they just perpetually clutter up the view making it harder to find only the comments that are not resolved issues.

@agibralter

This comment has been minimized.

Copy link

commented Jul 22, 2015

Yet another 👍 for "resolve comments." Sometimes it's nice for a PR comment to callout good code that shouldn't change. At the same time, it's nice to use comments as a to-do list... so those "nice job!" comments (though nice as they are) linger.

@nathangoulding

This comment has been minimized.

Copy link

commented Jul 23, 2015

+1 on the ability to resolve comments. Sometimes the comment will get resolved in a different location than the original comment was made in. Sometimes the resolution is to do nothing. In any case, "unresolved" comments are really important to understand on large PRs, otherwise PRs can (and do) get merged prematurely.

@dbrans

This comment has been minimized.

Copy link

commented Aug 11, 2015

+1 for a "resolve comment" feature and to view a list of unresolved comments.

@jperry

This comment has been minimized.

Copy link

commented Aug 18, 2015

+1 for "resolve comment".

@jmeridth

This comment has been minimized.

Copy link

commented Sep 12, 2015

👍 for "resolve comment"

@cfebs

This comment has been minimized.

Copy link

commented Sep 17, 2015

👍 resolving comments!

@whut

This comment has been minimized.

Copy link

commented Oct 15, 2015

Have you hear about Gerrit? It is standalone Git hosting and code review solution. There is also GerritHub, a service that provides Gerrit integration with GitHub pull requests,

Here is how the issues you pointed out look like in Gerrit

  • Mark comment as "resolved" - there is possibility to replay "Done" to any comment, but there is no possibility to see "which comments have not been addressed", btw I saw that in Atlassian Stash and Upsource, nice feature
  • Reply to a comment. - works in Gerrit really well, but the "comments thread" is tied to a particular "file version", so the "thread" does not "flow" to the updated file
  • Explicit "approve/block" life cycle. - works in Gerrit perfectly: in Gerrit you "vote" on a change, for example +2 "lets merge it", +1 means "I like it", -1 means "I do not like it", -2 "Do not ever merge"
  • Incremental diffs - that is what sold me to Gerrit, you see diffs between old and updated commits! The only issue here is when updated commit is also rebased onto new commit on "master", then you see both changes introduced in this updated commits and changes introduced by the new commits on master.
@danepowell

This comment has been minimized.

Copy link

commented Nov 12, 2015

+1 for resolve comments

@andersthorbeck

This comment has been minimized.

Copy link

commented Dec 15, 2015

+1 for the 1st, 2nd and 4th point, these would all be highly valuable to me as well.

@DaleShipp

This comment has been minimized.

Copy link

commented Jan 8, 2016

+1 for resolve comments and reply

@bblay

This comment has been minimized.

Copy link

commented Jan 8, 2016

Any news on this? With automatically folded comments we can't distinguish between those that are resolved and those that are not. This has been a problem for years. Could we override the folded status of a comment please?

@OlegoO

This comment has been minimized.

Copy link

commented Jan 18, 2016

Any news on this?

@kennknowles

This comment has been minimized.

Copy link

commented Mar 1, 2016

👍

@suhaasprasad

This comment has been minimized.

Copy link

commented Apr 20, 2016

+1 for manually resolving comments

@luosky

This comment has been minimized.

Copy link

commented Jun 3, 2016

+1

@cont-dmit

This comment has been minimized.

Copy link

commented Jun 16, 2016

+1

@megantracy93

This comment has been minimized.

Copy link

commented Jul 5, 2016

+1 for resolving comments manually; I don't understand why you would add "reactions" to comments before adding a "resolve" feature.

@dkubb

This comment has been minimized.

Copy link

commented Jul 11, 2016

It looks like someone added an issue for "Mark command as resolved" in the "dear-github" project: dear-github/dear-github#154

My assumption is that github people are watching this repo. Hopefully we'll get a few more reactions to help them prioritize it.

@rasmusgo

This comment has been minimized.

Copy link

commented Sep 21, 2016

+1 for resolving comments manually

@aboedo

This comment has been minimized.

Copy link

commented Oct 12, 2016

+1 for resolving comments manually

@Maartenvm

This comment has been minimized.

Copy link

commented Dec 21, 2016

+1 for resolving comments manually

@alk-jerber

This comment has been minimized.

Copy link

commented Jan 23, 2017

👍 for "mark comment as resolved"

@uramoto-isid

This comment has been minimized.

Copy link

commented Mar 15, 2017

+1 for resolving comments manually

@AmatanHead

This comment has been minimized.

Copy link

commented Apr 19, 2017

+1 for resolving comments manually

@ebuchman

This comment has been minimized.

Copy link

commented Oct 5, 2017

+1 for resolving comments manually

@typingduck

This comment has been minimized.

Copy link

commented Jan 15, 2018

+1 for resolving comments manually

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.