When I was interviewing GitHub employees for my book (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.
My team recently started requiring an implementation proposal on the issue that has to be approved by their peers. It saves a lot of wasted effort by exploring the problem domain collaboratively before development begins.
I’ve seen people suggest this and then the horror on other engineers faces that follows. In my opinion, the difference between a good eng/dev and a great one is that they recognize when the effort is too big to chew in one bite, too unknown to reason about clearly without a plan, and/or too many gotchas already exist in the codebase to make it easy.
The great developers I’ve known would pull back and think about the problem longer, draft up some plans and noodle on it with colleagues and I love that foresight.
The problem with a policy like this is that you’re slowing everything down for the sake of the few that might need it. Your team leads should be able to determine which ones should require the investigation and keep the team moving efficiently.
It shouldn't be slowing good developers down, it should just be changing the order they were already doing things. One of the most fundamental ideas I took away from my Software Engineering course in college was that catching errors early in the development process is more efficient than catching them later. It is a lot faster to grind out ideas in the planning phase than to churn during code review, because the development and code review processes are a lot slower than just replying to comments.
More importantly, when experienced developers formalize their reasoning for a proposal, less experienced developers can learn from it. That same knowledge becomes archived in a format that is easy to discover and consume in the future.
Someone suggested this on my team. I personally don’t like the idea because these policies often times lead to bureaucracy and then nothing getting released. It is not that I am against thinking ahead but if i have to in details explain everything I do, then more time is spent documenting than actually creating which is the part I enjoy.
But, do you believe the statistic that 90% of the cost of software is the maintenance?
If so, then the only way you can be a mythical 10x engineer is if you are willing to do things that really reduce the team costs. Everything else about 10x engineers is just mythology.
I think this is an example / practice of one of those things you can do for your team. Communicating clearly up front, being willing to discuss and alter poor thinking (everyone does it) is what turns individual contributors into powerful team members. If you just start with coding, you are missing all that part of it.
Documenting/communicating what you do is important but when it becomes a process it can really be detrimental. Every new feature won’t take the same amount of time and having to spent time writing down my approach for a task that will only take a few days will likely just limit me. That time could be spent on writing (more) tests which is also a form of documentation. If we say that 9/10 times a solution is good, the time saved not writing down the approach could accumulate to 1 rewrite. Basically for me it boils down to some of the agile values and principless:
- Individuals and interactions over processes and tools
- Working software over comprehensive documentation
- Working software is the primary measure of progress.
- The most efficient and effective method of conveying information to and within a development
team is face-to-face conversation.
> do you believe the statistic that 90% of the cost of software is the maintenance?
Hmmm. Apart from the very suspicious number (90%) I wonder what exactly it's meant by "maintenance". Because it could be bug fixing, adding or changing features, refactoring, server or db administration, etc. And what period of time is included in the "maintenance" phase: one year, ten, a century?
It really depends on the team. There are many cases in my memory where it made sense to code out the skeleton of the plan, especially at integration points and module interfaces, so that it's explicitly clear to other developers what you plan to do.
White-boarding doesn't really address the problem because some of the thorny issues really are substantive scalability or maintainability considerations. I've found it much easier to discuss with mocked code.
We tend to split workloads among teams based on timezones, so using placeholder PRs are required if you're handing your ticket over to another timezone for the night.
If your workplace is optimizing for your enjoyment, there is probably a serious problem afoot. The value of thinking ahead, to be clear, is that it enables you to "actually create" something that isn't ill thought out, or worse, of no actual use to any person. It also allows your work to be understood by other people, who might have to duplicate it, review it, or extend it in the future. It is indeed unfortunate that all of these legitimate demands should intrude on your time that could instead be spent spewing out code.
Have to disagree. Many places try to optimize for enjoyment so they can keep their employees.
There is a time and place for everything. If you a working on a project that takes months it should be planned ahead but if it only takes a few days you might not need. Let us not forget why we got rid of waterfall model.
> It was actually mandated for many government projects for a while
I have no clue about this, but I suspect that the government, rather than mandating a software development methodology, simply used to agree beforehand on what was to be delivered and expected to have it delivered at the end of the process. It's a constraint on how you sign contracts with external suppliers, not on how you write code- though I understand that the first influences the second.
In many industries as well as many government branches it still works just like that. They do not mandate a way of "writing the code", but software projects must pass gates and receive approvals at each stage before getting funding. They have to provide comprehensive analysis and architecture documentation that cover everything under the sun before a single line of code is written. And then, once a dev team starts coding, it has to go through the whole loop and if something specified doesn't make sense "on the real world". Or, more often, they just do the proper thing without telling anyone.
Interesting! Too young to have experienced waterfall model in a workplace. Are you saying it did not exists. If so what was the agile manifest a response to?
Frankly I don't know. The Manifesto itself is not very clear about it. I've worked in agile and non-agile companies, but none used a waterfall model. If anything, agile adoption always meant more process (stories, standups, planning, retrospectives, etc.) rather than less. Before or without agile, the structuring of work has been mostly the same, albeit without the formal process defined by this or that agile methodology (to be fair, scrum is the only one I've ever seen).
Writing down what you intend to do and verifying with your team it is sensible is not "the waterfall model". This "I just want to code" attitude is a major disease of young programmers and a leading cause of shitty software. Think ahead. Write it down. You are not a lonesome dove.
Thats a fair point, but I did not say that there should be no communication about the work you do. We all work differently and I think it is sensible to let each employee have some form of independence of how they want to structure their own work. If the feature is a few days of work let me work on it like I prefer and then I can submit with a nice description and thought process.
Let us also not forget about all the time wasted on getting your plan validated. Or how your validated plan is completely screwed only realized after you start implementing it. Pros and cons to each solution.
Again, there is no justification for just jumping in and writing code. Would you cut wood without measuring it first? Would you start soldering without laying out a circuit? Would you write an essay without an outline? You could do all of these things, but the results are guaranteed to be crap.
In my experience, communicating too much in the issue tracker tends to invite unnecessary discussion/bikeshedding with people who don't need to know the plan of attack. Our support staff are on our issue tracker, but not on our PRs.
I think this feature is meant to be a good middle ground rather than having one issue and one PR ticket. DO you want an issue with added patch set power, or a PR with better collaboration? They'd look pretty similar either way. They went with the latter.
Back when I was first doing XP style development, we did everything with trunk based development (TBD). As a rule, we pushed to trunk (master) every 20 minutes or so. Since we were all co-located (i.e. in the same time zone and working relatively similar hours) it meant that there would be a push to trunk every 5 minutes or so (frequency depended on the team size, of course). We read all of the diffs between trunk and our branch before we merged our code -- and integrated with any changes.
Probably most people won't realise, but it was in that kind of situation that "continuous integration servers" came into being. The idea was that after you pushed to trunk, the tests would run (and in those days we slapped your fingers if the test suite ran for more than a minute or two -- for reasons which will become obvious) and you would have a minute or two to fix any gaffs you made merging conflicts, etc. If you wanted to merge your code, you would wait until the CI server gave a green light, so if the tests were slow, it would be a major PITA.
The massive upside of this process was that you always knew what your colleagues were planning to do. You saw all their progress every 20 minutes! You integrated with their changes! If you saw a problem, you yelled over the cubicle wall and said, "Hey that won't work because I'm doing X" -- saving both you and your colleague hours of wasted effort.
TBD in this style is hard, but very rewarding. The main reason I don't practice it these days is because 1) We have continuous deployment and you don't want to deploy unfinished work 2) The teams I work on these days are distributed (and I'm 9 timezone hours away from their day). However, I really miss that immediate feedback and the seemingly effortless way in which everyone on the team is kept completely in sync. I've never found anything that even comes close (and I've done the "preliminary PR" thing many times).
That was with a very small team? Seems that if you get to only five people or so you'd already be spending most of your time understanding their commits.
In my experience, not so much? You'll need to worry if you're working on tasks that touch the same corner of the codebase, but larger teams tend to correlate with larger codebases, meaning that's not necessarily more likely.
That sort of XP style development also tends to come hand-in-hand with near-constant pair programming. So even if the conceptual limit is "5 sets of committers", that's still double the actual team size. And in practice, the cross-pollination that comes with swapping pairs every day or every few days means there's less of a cognitive load to needing to be aware of what's going on, since chances are either you or your pair already has context.
Indeed, code review shouldn't be architecture review. At that point it's too late and the friction/cost of revisiting the structure is often high. Early sign-off on broad strategy is great for productivity even if it takes a while to document the approach ahead of time.
One of my previous teams used to do something similar, but the initial PR would be for a change to the docs for the project. Would be great, because the docs would be updated to reflect any new changes automatically.
Is 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.
Well, sites like GitHub could be designed so the PR appears as merged from the beginning and then enabling the "merge" button, once more commits are available. Doing it this way also allows to naturally support merging a PR/MR more than once, which I don't know if it's undesirable, but it more closely aligns with what git supports.
Well 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.
The way I've done this before is to have a directory in the project called TODO. In the directory, I add a file with the same name as the feature branch I'm going to work on. I put a description of the work I'm going to do (and any TODOs if I think of them). I commit that and submit a PR with the title "DO NOT MERGE -- whatever".
Help 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!
* Signifies that the work is being done, preventing duplicate efforts.
* Shows people all the work being done in a single location, rather than having to hunt down forks and branches.
* Gives a nice UI showing the exact changes, without having to compare to specific commits.
* Gives a review interface, where people can comment on the code itself or even on specific lines.
All your other points still stand- you still create one branch per feature, check in frequently and push up work. If you explicitly want a review you can ask people for it (as you already do), but now they have a nice place to do it.
Right, this is a much better approach than such titles (or equivalent labels, which I've also seen), because it directly impacts GitHub's "ready to merge" checks at the bottom of the PR (without needing an outside bot/service for it).
Wouldn't this same person do the same for a Draft PR? i.e. if they're just reviewing things wouldn't they just set the Draft PR 'ready to review' and then review it?
Or maybe I'm confused by your answer, are people commonly reviewing PR's without being asked?
What the OP is saying is that someone can review a PR even when it's not ready by accident (not seeing WIP in title/tag), while with a draft, the reviews are disabled until its ready for review.
This basically is a better WIP system than before because it can prevent accidental reviews and wasting time of re-reviewing when it actually is ready.
I use and ask other team members to make draft PRs now (ad-hoc, basically the subject includes "WIP").
#1. PR triggered checks will fire
#2. You use the same tools for reviewing and can leave better comments
#3. I like PR comments to be detailed and explain the changes well for non-team members. Composing the draft message can help reveal items I need to make clearer or need to still do.
#4. It's clearer which branches/tasks are still being worked on, and how the work is progressing.
This works well on your own project, when you have permission to create branches... but what if you're trying to contribute to some other large and complex project? "Go look at my forked branch" isn't a great way to discuss a feature.
The 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.
If 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.
I've used sr.ht for some time and it's not that hard to work with e-mail flow. It's still not as easy (the main problem is setting up SMTP settings but that's done once). From my perspective most of the issues with git that GH solves is tooling, that same could be achieved with ML.
That said, having worked with git internals and models of repo access I'm pretty sure GH introduced forks to work-around the issues with repository control. Conceptually there is no need for forks, one could push to private branches of the target repo directly. Unfortunately git doesn't have tools for per-branch level controls directly. More details in https://github.com/rescrv/gitswitch
In a draft PR you can add a description and annotate lines of code with comments - making it easier for your peers. A branch can be harder to find and in your case you had to manually notify that you wanted them to have look at your code, whereas with a draft PR you probably get a notification.
So basically it's for the code competing interface? I guess that makes sense. I've never worked with a team that made good use of those, so maybe that's why I didn't think of it. Thanks!
It 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.
Well, 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:
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.
Do 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
Yes, I'm talking about coordinating PRs against multiple repos. Obviously, this isn't a git feature, this is something that Github would need to do, just like issues/webhooks/comments are not git features.
This is one of the reasons cited for monorepos, you can make a single atomic commit/PR that changes some API, and the consumers of that API.
As it is now, you end up writing a warning comment to merge some other PR at the same time and hoping your reviewers read it.
I think the way git supports this workflow currently is to have a super-repo containing all the other repos as submodules. You can bundle all those changes together as updates to the submodule references in a single PR.
I 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.
Gerrit 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.
I 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.
Yes, 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.
I 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.
Nope, 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.
Except 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.
This 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?
And 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.
Aren'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.
The 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.
That 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").
Love 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.
This 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.
Very 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.
This 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!
It’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.)
> 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.
I 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.
I 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.
This 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.
The 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.
I 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?
> 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.
> 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.
Merging into a single branch isn’t the definition that people like ThoughtWorks seem to use. They just talk about integrating into a shared respository, not specifically a shared branch.
The CI system then does the work for you to test that on top of master. You’re talking about developers doing things that the CI system is there to do.
CI only seems to make sense to me when talking about a single branch ("master", "trunk") into which all changes are frequently merged. FWIW, also Wikipedia describes it as "merging all developer working copies to a shared mainline", the latter being a branch, not repo.
Otherwise, you still might test indvidual feature branches on top of master, but that wouldn't prevent any conflicts between several of the pending branches. One could think of a system that continuously tests all possible combinations of all pending feature branches to spot any conflicts before actually integrating them. Not sure whether such thing exists, though, or whether it'd be very practical.
I often write just my database models, or just my API/schema first. It's typically syntactically correct, but entirely non-functional.
I'll sometimes open a PR for this with "WIP" in the title, "DO NOT MERGE" in the body, or without the "For review" label that we use.
These PRs serve to facilitate discussion, start knowledge sharing, and update other developers on my progress more clearly than a branch they might have to check more carefully. However I would definitely not want to merge one.
I love CI/CD, we do a lot of it and try to have small changes and feature flagging as much as possible, but this feels pretty useful in general for us. Yes it could be mis-used, but I don't think that's a reason not to have the feature.
It really does depend on what you're developing and what you're shipping. Even minutia like language and build systems influence what's a convenient way to manage in progress work. I'm much more tolerant of dead-code/feature flag branches in certain parts of my codebase than others even. Sometimes you know exactly what you're building, but other times you're trying to sketch out an idea to see what it might look like and to get buy-in for a feature. You'll need to decide if you want to put the drag of partial work on everyone else who's making changes or on yourself to keep up with HEAD.
It's a choice.
All that said I think this feature in github is minor enough that it'll make what people were doing already more convenient (UX win) but won't substantially change how people work. It's just not dramatic enough.
Yeah I can see that. Maybe the term "PR" can lead to misuse with things that would be better off integrated. I wonder, Perhaps a better name that does not imply that use case might make better sense.
