26 ms·
More code review tools
- danpalmer 11y agoThis is a great step in the right direction. I use GitHub for code review every day, and it has historically been very poorly designed for thorough reviews. These changes look great, I just hope that we get some sort of checking-off of review points, and accept/reject functionality.
- avolcano 11y agoI wonder if there's a set of API hooks they could expose to allow third party applications to handle more advanced code review techniques? I would totally understand GitHub not wanting to implement a full Phabricator/Gerrit review feature set, but maybe they could make it easier to integrate other services for it.
- moby 11y agoThat's a great thought. For my own edification, which third party apps come to mind? At present, the "official" collaboration tools in the GitHub integrations directory are here: https://github.com/integrations/feature/collaborate https://github.com/integrations/feature/collaborate
- ed_blackburn 11y agoI agree. I think there's a whole ecosystem there.
- cmsj 11y agoI don't understand it :) GitHub has become the de-facto place to host open source projects. Improving the tooling around code merging, seems like a no-brainer, and a huge potential win for the projects using it, especially the larger ones.
- erichurkman 11y agoI'd love a UI way to enable `?w=1` whitespace mode, especially for html/json/yaml files. I have a feeling a lot of reviewers don't know about it and spend an inordinate amount of time squinting to see what changes when it was a whitespace-only change.
- nathankleyn 11y agoSome similar option for enabling patience diffing as well would be amazing. Those two options in combination would certainly make a not insignificant amount of PRs much more readable.
- wclax04 11y agoI REALLY wish I could exclude certain file types from the view.
- tyingq 11y agoThis! ?w=1 has existed for years, with no UI way to access it. Compare two views of the same commit (found this by searching for any commit on github with the word "indent" in it) https://github.com/douglascrockford/JSON-js/commit/1e3869cb398ddf58d3d52efd735067093dc5bf3e https://github.com/douglascrockford/JSON-js/commit/1e3869cb3... (133 additions and 127 deletions) https://github.com/douglascrockford/JSON-js/commit/1e3869cb398ddf58d3d52efd735067093dc5bf3e?w=1 https://github.com/douglascrockford/JSON-js/commit/1e3869cb3... (30 additions and 24 deletions)
- xPaw 11y agoI have a userscript that does just that, however it needs to be updated to account for these new review tools. https://gist.github.com/xPaw/de6ee132a2e267ef6960 https://gist.github.com/xPaw/de6ee132a2e267ef6960
- mwarkentin 11y agoOn top of that.. when you're in `?w=1` view you can't leave line-by-line comments. :facepalm:
- alxndr 11y agoI created a reminder for myself so that every six months I write an email to GitHub asking for someone to create a user option for this whitespace setting.
- faitswulff 11y agoIt's interesting that you mention checking off of review points. I had begun to do that in an informal fashion by updating my pull request with notes in the form of a checklist: - [x] Refactor _ - [ ] Rename variable x - [ ] ... Now that you mention it, it would be very nice to have something like Google Docs's ability to mark comments as resolved.
- deleted 11y ago[deleted]
- danpalmer 11y agoUnfortunately that doesn't work for us for 2 reasons - 1. There's a data loss bug when multiple people edit the PR descriptions. I've had that as an open issue with GitHub for ~6 months, my team experiences it several times a week. 2. We use the checkboxes to assign and track reviewers, and since there's only one count of checkboxes, it would mess with our "2 of 5 complete" kind of metric for reviews. I'd like first-class support for the concept of people reviewing and accepting (or rejecting) so that it's taken out of the PR description.
- marcinkuzminski 11y agoThis is exactly how voting, and reviewers concept works in RhodeCode. You have accept/reject/under review states for each reviewer, and you have to make full voting in order to merge a PR.
- nightpool 11y agoQuestion: why do you use checklists to assign reviewers, when github has that functionality built in?
- jbrooksuk 11y agoBecause GitHub only allows you to assign to one person per issue.
- 11y ago
- numbsafari 11y agoI'd love it if there was a way for me to queue my comments before submitting. I often keep my comments in a separate textedit/nv window and then go back to put them in since I want to keep track of questions that arise as I'm reading, but I don't want to pepper someone with comments that would be resolved 200 lines later in the diff.
- Hovertruck 11y agoYeah this is my #1 feature request, and the main reason that a number of companies I've worked for/chatted with have switched to using Phabricator for their code review.
- jawns 11y agoCan't you just enter your comment into the text box but not hit the "Comment" button until you've gotten through the whole PR? I just gave it a shot, and it looks like you can have multiple comment text boxes open simultaneously.
- bpicolo 11y agoAnd yet it will still send <n> emails at the end, and you lose progress if you close tab
- BinaryIdiot 11y agoAs you enter your comments just don't click the "comment" button. Github saves the text anyway as you go. I don't know how long it takes to save the text (so maybe don't refresh or leave the instant you finish writing a comment) but in general they should all be there. Then you just have to scroll through and click comment on each one. Certainly not as easy as seeing them in a nice row for review but it's a workaround at least.
- mfonda 11y agoThis is my biggest feature request here as well. I'll often write a lot of small comments inline in the diff, and a bigger comment on the PR itself tying everything together. It'd be nice to be able to submit it all at once, with the ability to reference inline comments in my main comment.
- pearlsteinj 11y agoI've been pleasantly surprised at how fast Github has been moving since that critical open letter came out. For a giant company like Github they've been releasing developer tools very quickly.
- vdnkh 11y agoMakes you question what exactly they were doing before the letter came out.
- softawre 11y agoTaking naps in big piles of cash? Heh.. Enterprise-only stuff?
- asadlionpk 11y agoYes. They were/are heavily focused on Enterprise features.
- willchen 11y agoDoes anybody else feel like GitHub has released more features in the last month than the last 6 months? I'm not sure if it's just a coincidence with all the attention they've gotten on HN, but these improvements are much appreciated!
- morinted 11y agoIt was probably in reaction to the Dear GitHub letter. People were considering migrating away from GitHub and so they got their hands out of their pockets. We all benefit, though, GitHub becomes a better platform for us and they probably become a more successful company.
- grouseway 11y agoPeople who defended them should take note. Complaints can lead to action. Cheering for software companies is as useful as cheering for sports teams. If you go into defence mode whenever someone complains about your favorite VCS, OS, language, platform, editor, start menu, or whatever then you're probably doing it a disservice unless you're disputing factually incorrect information. Posting work-arounds and minimizing others people's complaints isn't helpful.
- WillAbides 11y agoI get what your saying, but the sports analogy might be a bit off. Most sports fans are most critical of their own teams and expect to lose every game.
- chrisweekly 11y agoHome teams have an advantage in most sports, and crowd noise is a factor. More relevant to your point though: encouragement and moral support are important to many OSS projects, where burnout is a particular risk. Maybe not applicable to github per se? /random thoughts
- smaili 11y agoDefinitely not a coincidence. Although it certainly begs the question of why this wasn't done earlier given the sheer amount of capital they have.
- alexwebb2 11y agoDid they kill commit-level comments? I can no longer make comments on a given commit - the entry box at the bottom is gone. Looks like you have to scroll all the way back up to the top, switch to the "Conversation" tab, and then make a PR-level comment instead of a commit-level comment. I hope I'm missing something here, because as it stands now it's a big step backwards.
- nightpool 11y agoThey removed full-commit comments on PRs, as opposed to line-level comments on either commits or diffs, or full-commit comments on non-PR commits, but I don't think anyone used those—I haven't even seen anyone use something that wasn't a line comment for a long, long time.
- alexwebb2 11y agoLine-level comments trigger an absurd number of notifications, and there's no way to batch them. We abandoned that idea within a day. PR-level comments lack any sort of context - there's no code at all unless you manually link it. Commit-level comments were a sane, reasonable compromise, and now they're gone. Your own workflow is obviously going to be different than mine, and it's good that you aren't affected by this, but hopefully you can relate to my disappointment with the way they've handled this. How would you feel if a product you were paying for suddenly dropped support for a feature you rely on without any notice or explanation?
- yxhuvud 11y agoNot being able to comment on the commit message is non-stop retarded though.
- eridius 11y agoI've used commit-level comments before, but it is admittedly not very common.
- majewsky 11y agoIt is very useful in work cultures where people don't do PRs all the time (particularly out of laziness or to do hotfixes), but you still want to comment on the change.
- hwangmoretime 11y agoCode review is something all of us do, but all of us do differently. Anyone know of any nice frameworks, articles, or blog posts for code review? I'm particularly interested in the case where knowledge transfer is a high priority in the code review. My current side project integrates feedback theory [1], to provide scaffolds and other cues to help and remind reviewers to give high quality feedback. Thus, my interest. 1: https://scholar.google.com/scholar?q=%22The+Effects+of+Feedback+Interventions+on+Performance%22&btnG=&hl=en&as_sdt=0%2C39&as_vis=1 https://scholar.google.com/scholar?q=%22The+Effects+of+Feedb...
- swanson 11y agoYou might find this talk valuable -- it's my personal favorite: https://www.youtube.com/watch?v=PJjmw9TRB7s https://www.youtube.com/watch?v=PJjmw9TRB7s
- owensims1 11y agoI wrote this high-level overview of how my team does code review, with an emphasis on knowledge transfer: http://engineering.zesty.com/code_review/ http://engineering.zesty.com/code_review/. Hope you enjoy.
- wpietri 11y agoFor knowledge transfer, my number one technique is pair programming. If there are more than two people involved, I add frequent pair rotation. The only time I've ever felt comfortable going on vacation is on pair programming teams. That's a sign to me that knowledge really has been properly transferred.
- Jemaclus 11y agoI think the biggest thing I want is pagination for extremely large commits. Any ideas if that's gonna happen?
- lukevers 11y agoMe too. The feeling when you accidentally click on a very large diff and your browser freezes for a few seconds is horrible.
- js2 11y agoIt would be nice if GitHub supported a Gerrit-inspired code-review process, where instead of having to choose between: 1) piling new commits onto the existing branch/PR, or 2) force-pushing and completely losing the old commits on the server You could instead push into a "magic" ref-spec and the server would retain the original commits, but also the rewritten commits, such that a PR can have multiple revisions. This is somewhat hard to explain unless you're familiar with Gerrit and its "patch set" workflow. Why? Because my preference is to rewrite history in order to address code review comments, such that what is finally merged leaves a clean history. But it is also valuable during the code review process to be able to look across the revisions to make sure everything has been properly addressed. The only way to do both, today, is to create a new branch and PR for each "revision". i.e. new-feature, new-feature-rev1, new-feature-rev2. Then close the original PR and reference it from the new PR. A bit tedious.
- nightpool 11y agoisn't this what the "view outdated diff" button does, or am I misunderstanding?
- kragniz 11y agoCan you diff between multiple outdated diffs? One of the really useful things in the gerrit workflow is to check what's changed since you last reviewed a patchset.
- js2 11y agoThat button only appears when you add new commits to the existing branch/PR. If you amend any commits and force push, the rewritten commits are lost forever on the GitHub side of things. You can only find them in your local ref-log at that point. Gerrit instead retains each rewrite of the "same" commit. It does this by requiring you to insert a "Change-Id: ..." footer into each commit message (it provides a repo hook to do this) and examines each incoming commit message to know whether that commit is a revision to a commit it's already seen.
- 11y ago
- deleted 11y ago[deleted]
- Negative1 11y agoNice additions. Can't help but feel at some point you'll be able to edit and compile code directly on Github (with some sort of compiler/CL backend). Has Github ever discussed integrating Atom directly into the site?
- alxndr 11y agoThere is a text editor; when viewing a Markdown file for example there's a pencil icon in the top-right of the (rendered) file display.
- artursapek 11y agoAwesome to see GitHub stepping to the plate with these updates. IMO the biggest thing missing in the PR's diff view is a git blame column beside the changes, so if someone is writing good commits I can glance through the entire diff and see which changes were introduced together and why. Just a feature request, and I know how annoying those can be on the receiving end. Great updates either way.
- netcraft 11y agoI wish it were possible to put comments directly on a line of code in any commit / repo outside of a PR. Sometimes someone wants me to review their code that they aren't submitting anywhere.
- timr 11y agoWe've been doing exactly that at Omniref: https://www.omniref.com https://www.omniref.com
- pizza 11y agoIf next to every file, Github also listed its filesize, I would know exactly what file to begin looking at when I encountered a new repo (to a good approximation -- in any case, I would be able to intuit better decisions with a combination of filenames, the information in hypothetical README.md's, and filesizes than just READMEs and filenames alone..) Maybe this doesn't scale or something? It's something I've felt necessary for a while though. Maybe their user testing has concluded otherwise?
- deleted 11y ago[deleted]
- jakub_g 11y agoI'll plug my work as always when talking about the topic: I wrote a small script for Chrome/Firefox that I found useful for reviewing big PRs on GitHub. It gives you possibility to expand/collapse files, mark files ok/fail in order come back to them later. It works with mouse and keyboard (though I've noticed there are some issues with tab order after GitHub upgraded their code, and small UI glitches, I'll try to have a look at them soon) It's a hack on top of GitHub so it needs maintenance every couple of months, but overally it does its job well IMO. I don't have much time to hack on it anymore, but community contribs are very welcome. I wrote some potential ideas for improvements as GH issues in the repo. AMA if you're interested. [1] https://github.com/jakub-g/gh-code-review-assistant https://github.com/jakub-g/gh-code-review-assistant
- guelo 11y agoSometimes when I submit a PR and I get a lot of feedback I get lost making sure I've addressed every comment. My #1 feature request would be being able to mark comment threads as resolved. The outdated comment feature sometimes works for this use case but mostly it doesn't.
- infogulch 11y agoI didn't see it mentioned but it looks like you can now 'react' to comments, a la facebook. So you could choose one reaction, say, "Horray!", and use it as a marker that you resolved it. Now I wonder if the commenter is notified on every reaction. The only thing that might make this more difficult.
- positr0n 11y agoThat would also be missing a filter to show all unresolved comments, which is a huge use case. (Or maybe you can do that, if you can it is not very quick and intuitive).
- cheshire137 11y ago> Now I wonder if the commenter is notified on every reaction. I don't think the commenter is notified about reactions to their comments.
- debacle 11y agoI'd like block-level comments and maybe even an in-commit commit function (the ability to commit to a PR branch while in the PR). That's all I really care about.
- fiatjaf 11y agoNo matter how much they do, people will still complain.
- willchen 11y agoI've noticed some open source projects (particularly Angular 2) follow this convention where they never actually "merge" the PR, but rather rebase it into their main branch and have a message in the commit to close the PR. The advantages seem to be that you get a cleaner git history, and you can keep all the "in-between" / WIP commits that tell the story. Is this done through an automated tooling or is someone manually rebasing it into master? It seems to be a really useful practice so I'm curious how people do it. It would be great if GitHub could offer this natively, as I think many power Git users appreciate the benefits of rebasing over merging.
- xkarga00 11y agoDear Github, can you please bring the search bar back (w/o the need to log in)?
- majewsky 11y ago+1 - I was so confused about this yesterday. For not just a moment, I thought they had eliminated global search entirely.
- eridius 11y agoThese are some nice changes, though there's still plenty of things I want GitHub to make better about code reviewing. One of these changes, and one that annoys me every single day, is when I get an email about a comment, the email doesn't include any context (e.g. it should include the previous comments on that line and probably the hunk as well). And when I click "View on GitHub" to see it in context, if the commit is now on an outdated diff, I get taken to a page that doesn't show the comment at all. It takes me to the Files view, but the Files view doesn't show outdated comments. If the comment is on an outdated diff then it really should take me to the Conversation view with the comment in question expanded.
- forgotpwtomain 11y agoI can't say I'm a fan. If I select a single commit, instead of giving me a list with a single commit check-marked, it gives me only that single commit and an option to 'show all commits'. So to change the commit you are viewing requires clicking 'show all commits' (waiting for them to load) and then selecting the other commit you want to view (which should just be a check box). Also it totally baffles me that these actions all require server-side requests. It's really a lot easier to go to the old commits tab, and to ctrl click an open tab for each commit then to use this new feature. At one point Github offered the best and cleanest UI of all the alternatives - but I doubt this will be the case for long at this rate.
- forgotpwtomain 11y agoI can't say I'm a fan. If I select a single commit, instead of giving me a list with a single commit check-marked, it gives me only that single commit and an option to 'show all commits'. So to change the commit you are viewing requires clicking 'show all commits' and then selecting the other commit you want to view. Also it totally baffles me that these are all server-side actions. Github for a long-time had the cleanest and most effective UI - but at this rate I doubt it will stay that way long..
- stormbrew 11y agoOnly related to review in a somewhat indirect way, but I really wish the commit view on github (as well as other git tools) had an equivalent of --left-only command line option to git-log. It's incredibly useful for viewing a high level of the history of a repo that uses merge bubbles, and lack of tooling around doing just that seems like that main reason people do things like squash their branches before merging to master (which is where I think it connects back to review).
- known 11y agoAre there any https://en.wikipedia.org/wiki/Software_design_pattern https://en.wikipedia.org/wiki/Software_design_pattern review tools?
- kdazzle 11y agoMy biggest pet-peeve with GH code review is that line-comments are automatically folded whenever that line of code is changed. So if the changes to that line didn't relate to your CR or if they didn't actually fix anything, then your comment will pretty much be lost to time. Plus, it would be nice to see the discussion around a particular line without having to go through the entire PR and unfolding each conversation to see if it's the right one.
- piotrkaminski 11y agoThen give Reviewable (https://reviewable.io https://reviewable.io) a try. :) Comments remain open until acknowledged / resolved and are automatically displayed at the nearest applicable line in every diff. Disclosure: I built the tool.