GitHub Community
Allow PR reviewers to comment on and suggest changes to unedited lines of code #4452
-
|
One of my favorite changes to GitHub's PR review experience was the introduction of suggested changes. I regularly use this feature in documentation PRs; suggesting changes really helps speed up the review cycle by allowing me to prepare a necessary change and allowing the PR creator to review and accept it rather than me commenting how we should modify the text. A major drawback to the current implementation, though, is that the GitHub UI does not allow reviewers to suggest changes to lines that are not directly affected by the PR. Hovering over a line in right next to changed lines gives an 'add line comment' button Hovering over a line that was hidden behind the "Expand up" or "Expand down" buttons does not display an 'add line comment' button Please allow reviewers to comment on and suggest changes to any line in the files under review. July 14, 2023 update: Unmarked @willsmythe's comment as an answer to this request as github/roadmap#347 is still open and on the backlog. |
Beta Was this translation helpful? Give feedback.
Replies: 78 comments 79 replies
-
|
Relevant. isaacs/github#1655
|
Beta Was this translation helpful? Give feedback.
-
|
Same from 2014: isaacs/github#284 Seriously, today I've just googled it again thinking to myself "maybe I'm just dumb and I don't know how to do it", just to rediscover all of those threads from 2014, and it turns out my past self went through the same process at some point because I've already upvoted all of them 😅 So here we go again |
Beta Was this translation helpful? Give feedback.
-
|
Yeah, hardly a day goes by that I don't want to comment on some line but the GitHub UI is like "nope, if you want to talk about that line go write out in English |
Beta Was this translation helpful? Give feedback.
-
|
How is this not a thing already? GitHub PRs are by far the worst code review tool I've ever used. This would make it a tiny bit better. |
Beta Was this translation helpful? Give feedback.
-
|
I just needed this feature today, I wanted to point out something that is related to the change but not part of it. |
Beta Was this translation helpful? Give feedback.
This comment has been minimized.
This comment has been minimized.
-
|
I also needed this feature today in order to comment on some code not changed by the PR but relevant to the PR's changes. |
Beta Was this translation helpful? Give feedback.
-
|
I'd think that it's a problem to do because code suggestions need to be retained, even as "outdated", even after PR author pushes more commits or even rebases and force-pushes the branch. Probably comments are linked to specific lines in the unified patch of the PR. |
Beta Was this translation helpful? Give feedback.
-
|
But it does allow you to comment on code that is near the changed code, up to 3 lines away. This seems to me like an arbitrary choice. The functionality appears to already exist for commenting on code that was not changed by the PR. Or are those nearby lines included in the patch file? In that case, I could understand why this is not trivial to implement. |
Beta Was this translation helpful? Give feedback.
-
|
That's my assumption. We're all developers. If something like this isn't already implemented, we can assume there was some structural choice made deep in GH codebase that makes this non-trivial to add in an efficient way. |
Beta Was this translation helpful? Give feedback.
-
3 lines is exactly the standard amount of context lines in unified diffs. Doesn't seem arbitrary to me. |
Beta Was this translation helpful? Give feedback.
-
|
What if you could set a line count somewhere in the .github file? |
Beta Was this translation helpful? Give feedback.
This comment was marked as spam.
This comment was marked as spam.
-
|
Completely bizarre that this is not possible. Case in point: a PR refactors a file from JS to TS. If a line has not been refactored and therefore changed, how do I point it out to the dev?? |
Beta Was this translation helpful? Give feedback.
-
|
GitHub public roadmap |
Beta Was this translation helpful? Give feedback.
This comment has been minimized.
This comment has been minimized.
-
|
Thanks for the feedback. We understand the need and are investigating the feasibility this quarter. 2021-12-19 update: Commenting on any line of a file changed in the pull request is on the roadmap for next quarter: github/roadmap#347. Please use #9099 to discuss commenting on unchanged files. |
Beta Was this translation helpful? Give feedback.
-
|
@chriskuech see "Answer selected by @michellemerrill" |
Beta Was this translation helpful? Give feedback.
-
|
Just started using this, and felt the same need. Thought it would be implemented by now, as this is a couple of year old. OMG |
Beta Was this translation helpful? Give feedback.
-
@willsmythe - That was for over a year ago. Is this feature that can prevent defects and improve quality really more important than roadmap items like these (from the roadmap) |
Beta Was this translation helpful? Give feedback.
-
|
@willsmythe I see github/roadmap#347 has been closed out. How can we follow along on if/when this will be revived on the roadmap? Looking for something I can follow in order to see when the work gets picked up and when it might be available. |
Beta Was this translation helpful? Give feedback.
This comment was marked as spam.
This comment was marked as spam.
-
|
++ |
Beta Was this translation helpful? Give feedback.
This comment was marked as off-topic.
This comment was marked as off-topic.
-
|
One of my favorite changes to GitHub's PR review experience was the introduction of suggested changes. I regularly use this feature in documentation PRs; suggesting changes really helps speed up the review cycle by allowing me to prepare a necessary change and allowing the PR creator to review and accept it rather than me commenting how we should modify the text. A major drawback to the current implementation, though, is that the GitHub UI does not allow reviewers to suggest changes to lines that are not directly affected by the PR. Hovering over a line in right next to changed lines gives an 'add line comment' button Hovering over a line that was hidden behind the "Expand up" or "Expand down" buttons does not display an 'add line comment' button Please allow reviewers to comment on and suggest changes to any line in the files under review. Replies Arcanorum image 0 replies davisking 0 replies ChJones-IDM 0 replies ayyadalaa 0 replies star-square 0 replies codesnik 0 replies whenmoon 0 replies willsmythe 8 replies @swythan Think "Security Linter" where one config change might cause another part of the configuration to become invalid (or to need a warning about the possibly unexpected cascaded change). @willsmythe I created a new discussion specifically about commenting on unchanged files: #9099. Please add any additional comments there (and upvote if this is something you need). @swythan - can you create a new discussion for your request about review bots and annotations? It wasn't clear if you were asking that unmodified, annotated files just be visible in the Files Changed tab, or for also allowing users to comment on them. @swythan I'll forward this to the team that was trying to do this. @AndrewMonteith @LittleHuman1 Write a reply |
Beta Was this translation helpful? Give feedback.
-
|
How can we tell the progress of github/roadmap#347, since Q1 is over and it is not possible still, just wondering if there is status somewhere, has it been started or not, almost done, done but waiting for official release, etc. |
Beta Was this translation helpful? Give feedback.
-
|
Looks like the status is
|
Beta Was this translation helpful? Give feedback.
-
|
This is nice!! But the rational at github/roadmap#347 is also applicable to not modified files |
Beta Was this translation helpful? Give feedback.
This comment was marked as off-topic.
This comment was marked as off-topic.
-
|
They moved it to "Future" milestone now, seems like AI has fully taken over, and DX is not as important now :( |
Beta Was this translation helpful? Give feedback.
-
|
Github is really disappointing compared to Gitlabs. And they're supposed to be the originators. |
Beta Was this translation helpful? Give feedback.
-
|
The credited originator of Git has unkind words for GitHub PR's/merges. I have different unkind words about the tooling and flow. That said, it could always be worse (and corrupt history every other week) |
Beta Was this translation helpful? Give feedback.
-
|
@jeffsaremi don't lose your hope on them man |
Beta Was this translation helpful? Give feedback.
-
|
Upvoted. This would be incredibly useful. |
Beta Was this translation helpful? Give feedback.
-
|
I just found myself needing this feature for the 1000th time today :) |
Beta Was this translation helpful? Give feedback.
-
|
Between this issue and the original it looks like this problem has over 3000 upvotes, making it the top thing your customers want solved. |
Beta Was this translation helpful? Give feedback.
-
|
I just did a review where this ended up being an issue multiple times. It's so frustrating to not be able to provide proper feedback when the line I want to comment on is right there, but just out of reach! Sorry for venting my frustration. :) |
Beta Was this translation helpful? Give feedback.
-
|
FWIW, here’s a simple hack I follow: I select multiple lines in the file (even if they are not part of the current PR). When you select lines, the URL in the browser updates to include the selected line numbers. If you copy this URL and open it in another tab, you'll see the selected lines highlighted. You can copy this URL and put in comment section of that particular file. |
Beta Was this translation helpful? Give feedback.
-
Yep was about to make the same comment but I'm glad I'm not the only one notices. |
Beta Was this translation helpful? Give feedback.
-
|
This is an important issue that many including myself have been thinking about for quite a long time. Today I came across a conflict that made me ask the following question: In what scenarios is the motivation behind the request for this feature not addressed by simply stacking another PR onto the source branch of the PR you're reviewing (even if it's an open source project and you're actually stacking that PR from your fork onto another third-party fork)? The PR OP can then either merge your stacked PR, cherry-pick, or whatever else they want but you both get a proper patch diff and you can of course edit anything in the stacked branch and then (perma)link to the proposed changes in your review comments. Apologies if this has already been proposed above and I missed it. |
Beta Was this translation helpful? Give feedback.
-
|
While possible, this is so far removed from the normal code review process to be a really viable thing for (what I would consider) normal workflows. Problems that make it high friction (non-exhaustive):
Maybe a button to "suggest changes" that opens up the PR's source tree as a (potential) branch on your own fork would simplify things, but all in service of a workaround to leave a single comment? Doesn't seem worth the extra effort on Github's side to me compared to the core request here. |
Beta Was this translation helpful? Give feedback.
-
|
Also, in the workflows I deal with PRs have to be justified and the code is owned by certain people. So a new PR means I am making changes rather than the person who was working on the original issue, and this new PR has to be attached to a tracked work item. All I really want to do it mention to the person making changes already that they should be making (or considering) another related change on a line they haven't already touched. |
Beta Was this translation helpful? Give feedback.
This comment was marked as spam.
This comment was marked as spam.
-
|
Yet another missing GitHub feature in the core code review product that Gerrit has long supported for over a decade. |
Beta Was this translation helpful? Give feedback.
-
|
+1, ReviewBoard has this feature, its very useful |
Beta Was this translation helpful? Give feedback.
-
|
GitLab has this feature and it has been critically important in MR reviews there. Unfortunately, we also have some repos in GitHub, which means PRs also need to be reviewed there for some things. It has been very limiting to not have this basic feature available in GitHub that is available in GitLab. As an example, I recently reviewed a PR in GitHub where a change was made to a certain line in a file. I added a suggested change with important updates to that line. That update then required making changes to another line in the same file that wasn't already changed in the PR. I couldn't add a suggested change or comment to the unchanged line, which was incredibly frustrating. When that happens, especially in PRs that require multiple line changes across multiple files that haven't already been changed in the PR, it's MUCH more difficult to track and make sure those lines are properly updated when you can't add comments to unchanged lines that need to be resolved before a PR can be merged. This is a feature I need to use almost daily, which I can use in GitLab, but not GitHub. I much prefer the merge request review experience in GitLab and I'm regularly frustrated every time I need to do a review in GitHub. This is one of my biggest sore points in using GitHub (though certainly not the only one), and it has me hoping our team can eventually move all repos to GitLab, where the code review experience is so much better. |
Beta Was this translation helpful? Give feedback.
-
|
even Atlassian introduced this feature in bitbucket. its been around 3years i added a comment here to add this feature. don't know what the GitHub team is thinking. |
Beta Was this translation helpful? Give feedback.
-
|
Probably fixing this won't help with getting a promotion. It has nothing to do with AI, or some other shiny new feature that has nothing to do with improving the core code review workflows. |
Beta Was this translation helpful? Give feedback.
-
|
I just ran into this issue today, and would like to request eyes on this. It's an awkward workaround to have to make a general comment that says, "go look at this line over there and make such and such a change because of what you did over here." |
Beta Was this translation helpful? Give feedback.
-
|
Beta Was this translation helpful? Give feedback.
-
|
You just made my day. Thanks! 🎉 |
Beta Was this translation helpful? Give feedback.
-
|
That's a good start - but why must a request for a change be limited to a changed file? think partial classes. Trying to mark where something should be put. Or 'if you do this change, all this other code can be removed'. |
Beta Was this translation helpful? Give feedback.
-
|
@brettshearer See #9099 for that discussion. |
Beta Was this translation helpful? Give feedback.
-
|
Great, now multilines suggestion are completely broken everywhere 👍 See #176894 |
Beta Was this translation helpful? Give feedback.
-
|
Shipped a few weeks ago! 🎉 See the announcement Commenting on unchanged files is something we're still exploring, so stay tuned! It's a little more complicated because it requires letting users browse the full branch in order to choose a file. |
Beta Was this translation helpful? Give feedback.
-
|
It broke multilines suggestion everywhere (see #176894) |
Beta Was this translation helpful? Give feedback.
and others
Shipped a few weeks ago! 🎉 See the announcement
Commenting on unchanged files is something we're still exploring, so stay tuned! It's a little more complicated because it requires letting users browse the full branch in order to choose a file.