On Pull Requests(cramer.io) |
On Pull Requests(cramer.io) |
Recently, I had to go through a full code review back on GH pull requests, and it felt like pulling teeth in comparison. They're fine for interacting with contributors to an open source project, but compared to working with a tool like Phabricator that's built for a code reviewer's workflow (and for teams of engineers working together on a project), they just don't hold a candle, in my opinion.
https://github.com/jpetazzo/gunsub
tl;dr: automatically unsubscribe from Github issues that you are not directly involved in.
So, you can get a notification for every issue or pull request that opens in the project or organization that you are a member of, as usual, but you don't get bombarded by every notification, e-mail and status update for the duration of the life of that issue, and associated follow-up e-mails. If you want the notifications, just get involved in the conversation.
I am sure Phabricator is very good, and I'll check it out, but I don't have to sell my boss GitHub, he's already sold on it. We just need to get on feature branches and pull requests (at least if we're going to grow.)
Also, the ability to set up Herald rules to do any notifications you want, like 'add me to CC for any code review that touches this file path in any of these repositories', 'set up an audit whenever someone commits changes to the master branch of this repository', etc. It has a ton of flexibility for integrating with other services, like allowing people to authenticate through their Google account, GitHub, Twitter, JIRA, Disqus, Twitch.tv, Mozilla Persona, and obviously LDAP as well, or normal Phabricator-specific username/password authentication.
It's really insane how much integration there is, how rapidly the development is progressing (we found a bug on a Friday and it was fixed by Monday afternoon), and how responsive the dev team is (in #phabricator on Freenode).
All that for free. Pretty great.
Did you consider whether that might've come from having pull requests that are simply too big? I've observed that especially neophyte developers tend to make commits and pull requests that are too big and conflate too many issues into one, that are be very handlable once split into smaller logical units.
In my experience, pull requests just aren't the right tool for the job at the moment. That's not to say they don't have their place, but there are definitely superior tools out there.
To me this looks like a typical attempt to solve a work process problem via technology. Using any code review tool doesn't help if you don't get code review done right. If you filter notifications based on rules in the tool or let the developers do it via email filtering is basically a question of philosophy. If you look at a Diff that way or another also doesn't change much about the review process in itself. Don't solve process questions with more software features!
IMHO getting loads of emails is great. I can filter by myself what is interesting to me right now. And worse than the one or two uninteresting emails every quarter is when I don't get the information I need. A non-engineer middle manager is allowed to complain about email overload in my eyes. A developer should be able to handle pure text.
And while we are at it, I also believe that less is more. No matter which repository hosts my code, I do code reviews on my computer with my own developer tools. I'm not even sure how the current version of GitHub or Bitbucket diffs looks like. These websites are basically the shell scripts and disk space that are used to backup my code and allow other people to access it. If a developer wants to do a code review he can also decide on his tools. The rest is just email and "git merge" (--no-ff). But that's just my opinion.
Of the hundreds of repositories we have going internally, the largest is our main GitHub codebase. We have between 80 and 100 developers committing to that one repository every week. This includes something like 160 commits per day across something like 30 pull requests, every day. A lot of us have switched to using the Notifications tab over email (though some use email filtering and others use both). That tab groups all your notifications by project and you can both ack and mute threads easily.
I tend to go to Notifications once a day or so and manage everything - mute the stuff I don't need to hear from again, unwatch repos where I don't need every notification, see where I'm mentioned, etc. The user and team mentions that this author refers to as a "hack" are in fact a very powerful notification feature. If we need someone from a specific team to review something, we mention them or their team. I find it much more powerful and flexible than the more rigid assignment system of other tools, though you can also assign people to the issues associated with a PR in GitHub as well.
The Issues guide on our Guides site is a great overview of these systems and how to use them well for even large teams.
I prefer the workflow for sending patches on mailing lists: Email patch, get feedback, make new commits to address feedback, rebase (important step), repeat.
I'm honestly curious. PR are a tool, you can misuse any tool.
Also, you mention doing code review with your own tools on your own machine but I don't think that's the use case being discussed by the OP. It sounds like you're discussing reviewing the code to decide if you should pull it (since it's happening solely on your machine). The OP though is talking about a discussion of the code that happens between 2 or MORE people. Tools help that.
Being able to mark specific lines with comments or questions and have a back and forth between the submitters and reviewers doesn't seem like something easy to do if you're doing all the code review on your local machine.
Take a look at some chromium code reviews. Here's one with about 7 people participating
https://codereview.chromium.org/231133002/
And you can see inline comments, for example here's a few
https://codereview.chromium.org/231133002/diff/120001/cc/ani...
Is there a good way for that to happen offline on your local machine to involve 7 people?
To answer the question about the >100 emails: Yes. From nearly everywhere I can I pull each and every email and then filter via filter rules and google's priority inbox (which you can mimic with open source tools as far as I've read). I seriously have only one or two emails more to handle than I want to in a span of three months. Sometimes I want an email with a specific topic, sometimes nearly everything a specific person writes or says, and it all gets piped and filtered quite well.
That's the awesomeness of pure text. There are not many limits to what you can achieve by having simple and well understood text formats. On the other hand the messaging in these programs is limited to what the program was meant to do. What a Phabricator filter rule is not able to do you are never able to do with Phabricator notifications. But emails where not meant to be sent and received in today's huge amounts, yet it's not a problem that can't be solved. Even if it is an open source tool that you install on your own server it's quite hard to make it do something different.
* Works with git. There isn't any VCS abstraction so if you want to use it with mercurial or whatever you're out of luck.
* The review tool (typically) owns the repository that code is pushed to (obviously this is a bit different when integrated with GitHub). Starting a review is typically something like "git push critic branch:r/branch".
* Because reviews are just branches you get all the features that the VCS implicity provides but patch based systems have to replicate (e.g. interdiffs) for free.
* Because reviews are just branches, multiple people can push to the same review so if you want e.g. a database expert to make the database changes for a particular change that is otherwise mostly written by a UI expert, that's possible.
* Assignment to reviewers is based on filters i.e. the tool picks the right reviewer for each change. There may be more than one reviewer for different parts of a single review request (e.g. where you have different owners for the database and ui code and a patch touches both those parts will be reviewed by different people). Reviews can be assigned to specific individuals when required.
* Review progress is tracked per commit and per file, so you can do long reviews in pieces rather than all at once.
* When issues are raised on particular lines of code it optimistically assumes that any subsequent push that touches those lines addresses the issue. If it wasn't addressed the reviewer reopens the issue. This works surprisingly well in practice.
* Rebases of the review branch are handled through "equivalent merge commits", which means that you retain all comments and progress from before the rebase.
* the UI is generally very "engineer designed" and won't win any beauty contests. But it has essential features that some other tools inexplicably lack, such as side-by-side diffs and syntax highlighting.
http://screenshots.gitsense.com/intelligent-code-reviews.htm...
There is a Smart Attribute that has been programmed to track who has approved/rejected diffs at the file and directory level. You can find the source for this attribute here:
https://github.com/gitsense/smart-attributes-for-code-review...
There is an attribute that has been programmed to run Google's style guide lint tool against C++ files in the code review. You can find the source for this attribute here:
https://github.com/gitsense/smart-attributes-for-code-review...
And there is an attribute has been programmed to generate an activity feed. You can find the source for this attribute here:
https://github.com/gitsense/smart-attributes-for-code-review...
With Smart Attributes, you can implement whatever code review logic you want. And if you've created a pretty useful one, you can share it with others or sell it, if you think somebody will pay for it.
I would prefer having a branch to review rather than a single patch. When there are fixes, add a new commit to the branch and squash/rebase before merging to master. This way it would be easier to see what is happening during the development of a more complex feature with many rounds of review and fixing.
(note: we might not be using the latest version of Gerrit and our version control practices could be better)
https://www.atlassian.com/software/stash
Disclosure: I work for Atlassian
I'm also a fan of Gerrit having to option to enforce fast-forwards, something GitHub doesn't allow: it will always make a merge commit, even if there's only one commit and its parent is HEAD of the branch being merged to.
Are you saying you prefer to deny others the ability to directly interact with the same git structure you have in their git tools (by simply fetching from a remote), instead forcing them to apply a patch from an email and hope everything goes right? If so, why would you do that? This seems like it's a little idiosyncrasy you have that has little to do with the actual topic at hand.
Sounds like your problem isn't with pull requests, but with neophytes you have experienced in github projects you looked at, who do not rebase and force push.
In the majority of projects (excepting those run by neophytes) i participate in, whether on github or off, there has always been a clear line of pull requests required to be clean, meaning not only should the commits not contain cruft-cleanup, they also have to be on HEAD, contain Changes-file adjustments, contain tests, etc. When patch makers get these instructions and force-push that kind of thing, it's a breeze for maintainers to do a fetch, then compare their local marker on the requesters branch with the new state of the remote branch.
Having a requester go "no, i only send emails" in that situation would not be constructive.