10 ms·
Introducing draft pull requests
- markovbot 8y agoI'm assuming this is a response to GitLab's concept of a WIP merge request.
- shaki-dora 8y agoWell, I guess there’s nothing wrong with Github replicating good ideas, considering Gitlab is pretty close to an identical clone.
- techntoke 8y agoExcept GitLab has an open-source self-hosted option and an entire DevOps pipeline with Kubernetes integration and many more features features than GitHub. They also provided free private repos long before GitHub.
- markovbot 8y agoNope, didn't mean to imply there was! GitLab certainly seems to have replicated a lot of the good ideas from GitHub, I just wanted to point out that it now seems to be going the other direction as well.
- dkhenry 8y agoI don't know if I would call it GitLab's concept. Every place I have worked at that has used github has had the idea of a Work In Progress pull request. Its a pretty natural concept for an multi-member development team. So much so that all of the places I have used it have written explicit CI checks that would check for a label on the PR or the title to be in a given format and would fail to make sure you couldn't merge the branch until WIP was removed.
- raphlinus 8y agoGerrit has had the concept of "draft" for a long time. It's also better at multiple "patchsets" as a "changelist" evolves, some of which can have rebasing without needing a forced update. On the other hand, it's somewhat hacky the way it layers on top of git, requiring a "change id" to link the different patchsets together. Much of the way it works came from trying to deliver a similar experience as Perforce. Disclosure: worked at Google, have used both systems a lot.
- hipnoizz 8y agoI found a small inconvenience of maintaining ChangeId (usually handled automatically by the git hook) a very small price to pay for all great features provided by Gerrit. I know that Gerrit team investigated few times a better way to attach this metadata to commits but so far it works this way. What other 'hacky ways' you mean? 'Magic' refs/for/* branches can be a bit weird at beginning. On the other hand introducing Gerrit 7-8 years ago to my back-then employer made me understand the Git model much better. Anyway, e.g. IntelliJ IDEA plugin for Gerrit should alleviate almost all pain points (if someone stick to JetBrains products). Were you working on Android/AOSP? I always assumed that this is the primary project where Googlers have a chance to work with Gerrit. I thought that Google uses some in-house CR system, as mentioned by https://www.gerritcodereview.com/about.html https://www.gerritcodereview.com/about.html.
- raphlinus 8y agoYes, Android then Fuchsia. I agree it's good, overall I prefer it to Github, but by "hacky" I mostly mean it's very much out of step from the way the rest of the ecosystem works. This is much more true for repo / jiri (which is Google's own spin on the submodule problem), but includes change ids.
- mikece 8y agoI doubt it. I've heard interviewees for years on podcasts talk about opening a PR as a means to discussing something long before the code is actually ready. Seems like this is just a feature following a common though informal common practice.
- jillesvangurp 8y agoNice, I tend to like having pull requests created early so people can follow what is happening and give early feedback.
- wooly_bully 8y agoYeah, my normal Gitlab workflow is: 1. Start a branch with a first commit 2. Open a PR and write details / background 3. Mark WIP 4. Eventually remove WIP and add someone as reviewer. Glad to see this is available now.
- systemtest 8y agoIn my team we use the pipeline to automatically create the merge request after the branch is pushed.
- zozbot123 8y agoYo dawg we heard you like merging so we put some pull requests in your pull requests so you can merge while you merge!
- house9-2 8y agoI just use a WIP label on the PR, keep it simple.
- felixfbecker 8y agoThat will send notifications to all code owners, which a draft PR will not
- RyanCavanaugh 8y agoDoesn't always work - if you're not a write-access collaborator on a repo, you can't add labels.
- WorldMaker 8y agoThat requires social knowledge of reviewers to not merge WIP stuff (and/or avoid missing WIP labels/notes). These draft PRs make it much more obvious in the "Merge Checks" at the bottom of the PR by blocking the PR from merge until no longer a draft (marked "Ready for Review").
- peterwwillis 8y agoThis blog post really doesn't sell this feature well. How is this any different at all from me just putting "WIP" in the title of the PR? Ok, so it prevents merging a WIP PR - who would do that?
- bswinnerton 8y agoIf you use the `CODEOWNERS` feature, it won't auto-ping anyone until it comes out of the draft state.
- peterwwillis 8y agoAnd that's useful (if you use CODEOWNERS) - but just having a checkbox that says "don't ping the CODEOWNERS" on all PRs would have the same effect without needing a different type of PR.
- deleted 8y ago[deleted]
- michaelmior 8y agoAren't "PRs which should ping the CODEOWNERS" and "PRs which shouldn't" two types of PR anyway? Personally, I think it's useful calling it a draft and preventing the merge anyway.
- tln 8y agoThe differences seem minor to me. I create draft PRs all the time and probably won't bother with using this feature, because I usually open PRs using the `hub` tool or via VSCode, and only rarely via GH web site.
- tln 8y agoThat being said I must say I appreciate all the QoL features Github has been adding.
- rhinoceraptor 8y agoIt would also be nice to have a way to link multiple PRs together so they can be merged simultaneously. A ton of people are working across multiple repos which makes reviewing and coordinating changes really annoying.
- morley 8y agoWhy not have everyone create their PRs against a feature branch instead of master, and merge the feature branch into master when the project is done?
- rhinoceraptor 8y agoI don't mean multiple PRs for the same repo, I mean multiple PRs across multiple repos.
- no_wizard 8y agoWell, if you practice trunk based development, (which at most, is short lived branches that aren't really suppose to last beyond 72 hours for development purposes), its not always best practice: https://trunkbaseddevelopment.com/ https://trunkbaseddevelopment.com/ The heart of this being this point: sometimes the workflow of the team demands different ways of thinking about it, and its nice to have that flexibility. Edit: worth noting also, this does something for those of us using this methodology (I am a big promoter of trunk based development with ruthless purging of short lived feature branches) Opening WIP PR allows everyone to see several things: 1. what issue is being worked on 2. that it is claimed by someone 3. When that issue was started to being worked on 4. The intentions in solving the issue (if done right) Those are real wins. I wonder if the hub client will get a open draft PR option soon.
- jiveturkey 8y agoDo you mean link multiple PRs across multiple repos? You can already merge multiple commits together in the same PR, in a single repo, of course. via branches. If so, this can't be done within git, since the overall container is a single repo.[1] There is no atomicity or even coordination between repos. You can get loose coordination though via integration with an issue tracker like jira. The tracker links multiple PRs across multiple repos into a single meta-merge. But you aren't going to get "simultaneous merge" in the sense of a single atomic cross-PR and cross-repo commit. [1] One of the things that is awful with github [not git itself], is that all your controls such as commit rules, privileges, visibility, have the granularity of the entire repo. Even though awful, this is the right approach considering that many git restrictions are per-repo, eg branches. vs perforce, eg
- xrd 8y agoWhen I was interviewing GitHub employees for my book (https://buildingtoolswithgithub.teddyhyde.io/ https://buildingtoolswithgithub.teddyhyde.io/) one of them made a surprising comment. Early on, his GitHub teammates told him he should create an empty PR with a plan of attack before writing any code. I thought it was a brilliant idea and most teams don't do this. Smart of them to make this a separate feature instead of just bolting on WIP.
- darkerside 8y agoIs there actually a way to do this, if there is no code difference between your new branch and the base?
- NickBusey 8y agoWell I personally like to start by writing the documentation first. That way your documentation changes become essentially your 'plan of attack' rather than having to write it out explicitly.
- sjburt 8y agoIn my team we'll just create a README or TODO and make a PR with that.
- nmjohn 8y agoIIRC, you just need a commit different in order to open a PR with github. To make an empty commit: git commit --allow-empty -m 'Empty commit so I can open a PR'
- darkerside 8y agoI thought Github looked for an actual code difference in the PR creation process, but it seems you're correct! Thanks for the tip.
- jolmg 8y agoIs that even necessary? I would have thought that simply making the branch and pushing it while it's on the same commit as the branch you're branching from would be enough.
- creyes 8y agoThis is rad. I usually like to put up a PR early and add a WIP label. This lets my teammates take a look at the early direction of whatever I'm doing and I can make changes as necessary. This is a small but really impactful feature!
- joeldrapper 8y agoI count at least 17,800 requests for this feature. https://www.google.com/search?q=%22WIP%22+inurl%3Apull+site%3Agithub.com https://www.google.com/search?q=%22WIP%22+inurl%3Apull+site%...
- ken 8y agoI count 17,800 cases where someone accomplished the same thing by typing 3 letters. I've done this many times myself, and I hope nobody was counting those as a feature request.
- yingw787 8y agoI love this! I draft pull requests as an ad-hoc branch management scheme at work with a [WIP] prefix, and reviewers are notified when I make a change (small team so not too spammy). When it's ready I update the summary and remove [WIP]. There might be a better way to do it (and I'd love to hear suggestions), but this is how I do it right now.
- fyhn 8y agoThis is good. We have been emulating this with either a "don't review yet" tag or by opening a pull request without setting reviewers, but it's clearly better to have support for it built in. More people will use it, and I don't have to train people in our particular way of indicating not-ready pull requests.
- reificator 8y agoThis will be helpful for the cultural clash that can sometimes happen when a new contributor tries to do something like this and the maintainers expect all PRs to be immediately merge-worthy.
- lowii 8y agoWhy? Aren't all merge requests a WIP until they are merged?
- fyhn 8y agoThe difference is whether the author of the pull request considers his code not ready for review or not. Both are work in progress but they need different treatment.
- jedberg 8y agoHelp me understand something -- right now my workflow is to create a new branch when I'm working on a new feature. One branch per feature. I check in frequently and push up so my work is saved. Sometimes I ask people to look at the code in progress when I want opinions on architecture or just overall code quality. When I think the feature is ready for merging, I'll create a pull request. How does a draft PR help me? What do I get with a draft PR that I don't already have? I assume it's a good idea, and everyone here seems to think so too, so I think I'm missing something obvious. Edit: Lots of great points below, mostly pointing out features that I don't really use a lot, which is why I didn't see the improvement. Thanks everyone!
- shaki-dora 8y agoThe blog post seems to specifically answer this question far better than anyone here could. It essentially formalizes a workflow like the one you describe. Remember than anything GH does “can be done by email”, to cite some early commentary. But collectively, the ease of use, and defaulting to best practice, that GH introduced have vastly improved and enlarged the open source community. I know I never submitted patches in the 10+ years in the industry pre-Github. Now, barely a week goes by without at least one PR.
- jedberg 8y ago> The blog post seems to specifically answer this question far better than anyone here could. Clearly not, otherwise I wouldn't have asked. :) > It essentially formalizes a workflow like the one you describe. How though? What do I gain by doing it this way that I can't already get? What's easier now? That's the part I'm not seeing.
- DogLover_ 8y agoIf you are happy with your workflow you can continue as usual. In other work-cultures people might not like your approach which is why a draft PR can be helpful. Basically what this feature does is that it avoids having to point to a branch and tell your coworker that it is WIP that you want feedback on. Usually a PR will also show the changes directly.
- royosherove 8y agoI could be wrong, but I feel this actually enables the wrong kind of behavior we should expect from developers. We want fast, continuously integrated code that is merged early. Usually that means using feature toggles so that we can see compilation issues without the code being active by default. This feature signals "yeah it's OK to take your time and not merge stuff incrementally - here's a feature just for that since it's such an important part of your workflow". Yes, people have been doing it anyway but at least it felt a bit like a hack, like it's not supposed to work that way. Am I crazy, or is this pushing back progress made in CI coding practices?
- chrisseaton 8y ago> We want fast, continuously integrated code that is merged early For some projects maybe. Other projects want changes to be very carefully presented, considered and reviewed. > it's not supposed to work that way According to who? Is this how you do things at work? Ok, but other people work differently. There’s not authority in programming to set what we’re ‘supposed’ to do. > is this pushing back progress made in CI coding practices? Seems orthogonal to me. You can run CI on a branch. But overall, no not everyone’s development is a mad dash to get everything merged instantly and some want carefully created PRs.
- deleted 8y ago[deleted]
- benjiweber 8y ago> Seems orthogonal to me. You can run CI on a branch. Funny how some terms can be diffused enough over time to come to mean almost the opposite of what they originally meant. Continuous Integration is about integrating continuously. "the practice of merging all developer working copies to a shared mainline several times a day." i.e. everyone commits to master multiple times a day. Then tooling gets built to facilitate this way of working. People start confusing build tooling with CI the practice. Eventually that tooling gets used to support people working in continual isolation on feature branches. People start calling this practice CI even though there's no integration in sight. Of course true CI isn't applicable for all contexts. Draft pull requests look like a great tool for where CI is impractical.
- michaelwda 8y agoI literally saw someone ask for this feature on Twitter last week. VSTS/VSO/DevOps/? already has something similar. This seems like the kind of feature that probably takes longer than a week to build and test, so I'm guessing it was already in the works.
- aidos 8y agoVery happy about this. We recently switched to github and I asked my team to start their PRs immediately. Was pretty disappointed to discover that they needed something to commit first. Having a draft state will in itself be really useful.
- dpeck 8y agoLove it. I’ve had success using Issues in the past to do get implementation feedback, and have a “one pager” for whatever I/the team was working on at the time, but it didn’t feel like it stuck very well with junior team members. Having something like this that allows a little closer relationship between the plan and the code created for the plan is nice.
- winrid 8y agoThis is nice. I've always used the WIP tag but this is cleaner since it blocks merging.
- dvtrn 8y agoI can already see this coming in handy on our private team repo as I was not far off from revoking someone's merge permissions for abusing PRs, this will be a nice little way to 'jail' the offending party.
- gus_massa 8y ago> Also, if you have a CODEOWNERS file in your repository, a draft pull request will suppress notifications to those reviewers until it is marked as ready for review. I'm not sure. When I send a PR marked with PR I usually want to know the opinion of the maintainer/owner to know if it is going in the right direction and if it worth finishing.
- emersion 8y agoIs there a way to convert a regular PR to a draft PR?
- domoritz 8y agoI need this as well. I have a ton of PRs with WIP labels.
- meowface 8y agoLooks great. Any idea when it'll be added to GitHub Enterprise? This would be really helpful for my team.
- saagarjha 8y agoIt’d be nice to have some way to configure exactly how the “draft pull request” works, because there are a couple cases where I think this could be improved: * Notifying the maintainers might be useful in some cases. Often, I will file a WIP pull request to get preliminary review by a maintainer so that I know I’m going in the right direction. * Being able to convert a draft pull request into a full one: Useful if the original author has stopped responding and you’d like to merge in their changes (after tweaking them, etc.)
- KuhlMensch 8y agoHey niiiice, opening PRs early is definitely the way I work. This will definitely help with PR grooming :)
- Scarblac 8y agoWe have Github Team and I don't see the feature. Still rolling out?