Allow PR reviewers to comment on and suggest changes to unedited lines of code #4452
Replies: 57 comments 60 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.
-
|
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.
-
|
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.
-
|
Now Q2 2023 and still in 'open' status. Any updates Github? |
Beta Was this translation helpful? Give feedback.
-
|
Can you at least mark this as unanswered considering the request is objectively not accurately answered? |
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.
-
|
++ |
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.
-
|
Please do it, it's weird that Azure DevOps offers such a better reviewing experience. |
Beta Was this translation helpful? Give feedback.
-
|
It seems like the best place to push for this is https://github.com/orgs/community/discussions/9099 |
Beta Was this translation helpful? Give feedback.
-
|
As noted in that discussion's introduction, it's asking for a different feature. Of course, if you had that feature, you wouldn't need this one… |
Beta Was this translation helpful? Give feedback.
-
|
Been using this for ages in Azure DevOps. |
Beta Was this translation helpful? Give feedback.
-
|
It's really insane. I liked GitHub in general, but when I started using collaboration features in my new project I was shocked to see how inferior the whole UI and Workflow is to Azure, with PR reviews leading the way. I mean sure, I was cursing Azure at times while using it, but GitHub is a thousand times worse in so many aspects. |
Beta Was this translation helpful? Give feedback.
-
|
I want to share a workaround that may work for someone. While this is far from being ideal and doesn't work for all use cases, it's kind of okay if you're building some kind of automation and all you want is to suggest code changes in another file using the API. TLDR: It works in reverse of the code-suggesting feature. First, you commit the changes and then give the user the opportunity to automatically revert them. Here are the steps:
Again, it is very, very far from the ideal solution but it can work for someone who wants to automate code suggestions that have a high chance of being accepted (e.g., automatic code updates). |
Beta Was this translation helpful? Give feedback.
-
|
That's a neat thought, but still seems kind of hacky. Not sure if it would be really fully functional in the day to day business of maintaining a FOSS project. Would have to be seen in practice.. Are you using this as a day-to-day workflow of yours? If yes, might be nice to include that in a tool like https://github.com/magit/forge That's by the way how I found your comment. Was searching if the "suggest changes" functionality is covered in the Magit Github integration. At first glance, it seems that it isn't.. |
Beta Was this translation helpful? Give feedback.
-
|
Any progress on this? I find GitHub to be a good code-review and collaboration platform, and this feature would be incredibly helpful! |
Beta Was this translation helpful? Give feedback.
-
|
waiting for updates on that :) |
Beta Was this translation helpful? Give feedback.
-
|
Sometime we need to point out to a fellow developer where he needs to do the change, and not allowing us to comment on those specific lines because they were not modified is annoying. |
Beta Was this translation helpful? Give feedback.
-
|
Unfortunate that this is still an issue after almost three years. Seems such an easy fix to activate. Don't see how there is a technical blocker for this. If there is, would be good to verbalize why. |
Beta Was this translation helpful? Give feedback.
-
|
I think that would be perfectly acceptable, we just need GitHub staff to say this is the reason explicitly. Without that this might or might not be the case, we just don't know ☹. Considering the amount of "any updates?" comments, they have never said this, but if someone has seen an official response, please do correct me with a link! |
Beta Was this translation helpful? Give feedback.
-
|
It's far from obvious to me that it requires major changes. You can already comment on the context lines outside the hunk. They could always attach the comment to the nearest hunk (and prioritize earlier hunks if there were two nearest hunks). To be a really high-quality design they might want to adjust the logic for obsoleting comments when PRs are updated, but even if they didn't do that, we'd still be miles ahead of where we are now. |
Beta Was this translation helpful? Give feedback.
-
|
I agree it is likely not so difficult. Github already has to store the comments and all related metadata (date, etc) somewhere that is not in git. Therefore what we're asking here is to add a new type of data, "comment-on-line-not-in-diff", call it whatever, you get the idea. All it would take is 2 things:
The data to store is again the same, it's just the UI that has to figure out that if a comment is for a file that is not in the changed files list, then it has to go in that section, but even that design is just off-the-cuff, maybe a separate section is not even necessary. |
Beta Was this translation helpful? Give feedback.
-
The lines that provide the context to which a hunk is anchored are part of the hunk.
And how would that work exactly? Relative distances change over the evolution of a PR, especially with merges/rebases/etc.
With the size of GitHub, all edge cases will be hit almost instantaneously. What you consider really high-quality is IMO the bare minimum for such a feature (figuring out the life cycle of comments over the lifetime of a PR under whatever model you choose for saving comments). That and they need to ensure that the current hunk-based workflow doesn't break, and also that they're not stuck with an unmaintainable mess of two parallel comment-tracking systems and all the ways these can cross wires. |
Beta Was this translation helpful? Give feedback.
-
Then the existing feature doesn't meet the bare minimum standard. |
Beta Was this translation helpful? Give feedback.
-
|
How on earth is this still not a thing?? |
Beta Was this translation helpful? Give feedback.
-
|
In the GitHub Advanced Security video they're able to create a ````suggestion` with "Outside changed files" targeting line 16 of package.json in GitHub comments. I'd love to know how this is done.
|
Beta Was this translation helpful? Give feedback.
-
|
I would imagine this is (will be?) possible via Sarif fix. It's not (yet?) documented. Note that it is currently possible to "comment" (put a message on a range of text) on non-changed files via a Sarif upload. |
Beta Was this translation helpful? Give feedback.
-
Beta Was this translation helpful? Give feedback.
-
|
Does anyone at Github read these feature requests? Has Github ever implemented a new requested feature? |
Beta Was this translation helpful? Give feedback.
-
|
Dear Github team just remove the if condition that not allowing to add comments on unedited lines. It's creating a pain 😭 |
Beta Was this translation helpful? Give feedback.
-
|
I doubt it is such a simple thing. I suspect there are database entries for the bits PRs touch; this would mean adding database entries for any file that is commented on during the PR dynamically, not just at PR creation time. |
Beta Was this translation helpful? Give feedback.
-
|
Any update on this? It would be really helpful. |
Beta Was this translation helpful? Give feedback.
-
|
Still waiting for this option |
Beta Was this translation helpful? Give feedback.
-
|
I too would love to see this feature. My company just switched over to GitHub and I was shocked to see something so fundamental just not be possible, and it hurts my ability to give proper feedback. Please consider it! |
Beta Was this translation helpful? Give feedback.
-
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.
All reactions