I'm a Developer Experience Engineer at Camunda Camunda builds a process orchestration platform we have a booth, come visit! I work mainly on the infrastructure for our documentation And a big part of that is making sure the contributor experience for our docs is enjoyable
section 0: What even is a PR?
level-setting
aka merge request
(read title and description) summarize the changes you'd like incorporated
represented in a diff format
so we can talk about the code changes
and a reviewer can suggest changes
and in the end, hopefully, the changes are approved.
so that the owner can incorporate the changes
an opportunity to share knowledge 1. of our system 2. mentor each other 3. in case one of us wins the lottery
so we can recall when we introduced changes and why ... and that historical record is useful for anyone who works on this codebase, any time in the future not just the individuals who worked directly on the PR.
we'll talk about using pull requests specifically through GitHub
most of what we're going to talk about is applicable to other source control platforms
there are a couple notes of GitHub specific tooling
you're in the right place if that's what you're looking for
but more than GitHub, more than pull requests or merge requests, this is a talk about culture
and more specifically, asynchronous collaborative culture
also, shout out to olivia
section 1: Act with compassion
and kind? I don't think I need to explain this any further ...but I'll show you an example that suggest I do.
I mean maybe it's just me but raise your hand if you'd prefer to be a part of the first community raise your hand if you'd prefer to be part of the second community ...don't be a jerk.
And acting with compassion starts with what you can control which is yourself many people say "assume positive intent" but ironically that actually gives people an out to act _without_ positive intent so let's focus on what we can do 2 - even with the right intent, impact can be negative. Own your actions when things go wrong. 3 - call out bad acts, and on the other side, reward good acts acts, not actors, because they don't know they're doing something bad
I said this is a talk about culture, and I want to share my favorite analogy of culture as a community garden. When you find a good culture, it's like walking into a community garden that produces the best fruits and vegetables. The carrots taste delicious, and you don't want to leave. But the carrots don't grow themselves. The people in the community cultivate them. When weeds start to take over, someone needs to pull them. You can rely on someone else to do it, and still enjoy the carrots, but you can also contribute. Calling out bad acts in pull request interactions is a small but important contribution to the weeding of the carrots. It's critical to keeping the garden healthy.
section 2: Acknowledge power dynamics
Power dynamics have a huge impact on collaboration, especially in the interpretation of ambiguous communication. If you think "this doesn't affect me,"
Power dynamics appear in every relationship we have. You certainly know about them when you're on the weaker side of it. And when you don't know about a power dynamic in a relationship, there's a good chance it's because you're on the stronger side. They can be problematic, but we can also navigate them successfully in a team. Especially if we are aware of them.
and those multiple directions can conflict For example, I interact with my boss in my PRs every day. She's my boss - that's one power dynamic, in which I am on the weaker side. On the other hand, she looks to me for many technical details and decisions. That's a power dynamic in the opposite direction, and it also appears in the interactions we have in a PR.
The best way to navigate power dynamics is to be careful with words and choose them intentionally The weight of our words, especially when we're on the strong side of the dynamic, is incredible. Choose them carefully. ... And to avoid misinterpretation of words in a power dynamic be explicit instead of implicit. When something isn't that important, SAY SO. When it is important, SAY SO. Don't leave it up to the reader to interpret. Feedback from the strong side takes on a very heavy weight. That weight can easily elevate something that doesn't matter to something that is perceived to matter very much.
section 3: Choose curiosity over judgement
when a line of code doesn't make sense, it usually doesn't mean the author is dumb, it means you don't understand how they approached the problem when a change in a PR doesn't make sense, that's often also the case.
if something doesn't make sense or seems wrong, ask questions about it don't be afraid to take it to a synchronous conversation just make sure that any decisions that come out of that conversation, or discoveries, or anything interesting, makes its way back to the PR as a comment.
section 3: remove ambiguity
Back to our previous section about asking instead of guessing If something is unclear, get more information. Asking questions is most direct way to remove ambiguity. But here's the thing...
Collaborating through PRs is *inherently asynchronous*, and with that comes some *really great* things. We can *collaborate across time and location*. People can *respond when it's the right time* for them, because *right now might not be the best time. So everyone can *take their time before responding*. And that means we get their - best focus - best ideas - best self (pause) But there is *slowness* to asynchronous communication. That can be really *frustrating*. And that slowness is exacerbated by every round trip Every time someone needs to ask another clarifying question That's potentially another day before we resolve this conversation
I have conversations like this with my 12yo over text message where we say nothing for 10 messages back and forth until finally she gets to her point You can't have conversations like that between US and Europe, where any message might not be answered until the next day So to collaborate asynchronously, we need to remove ambiguity as quickly as possible. (answer: the sleepover, cousin muffin has skipped a nap and is unhinged)
And we need to remove that ambiguity as proactively as possible. by communicating effectively ... this is probably a non-controversial statement and also very nebulous we're all trying to do this in a lot of ways here are some things to focus on.
Assumptions, context, conclusions...all of these things can easily be misinterpreted. State them explicitly.
Compassionately, obviously, but say what you want to say. An example: a few years ago I started doing this thing where I wanted to be extra friendly and considerate in my PR reviews
So no matter how important I thought a change was, I'd phrase comments like the top Extremely passively and indirect But what I really meant was the bottom - I think we should make this change. The top can be fine if you really don't have an opinion, just make sure you're saying what you mean.
frustratingly, this kind of documentation exists solely to point people to after they've proven that they didn't read it but it's still important to have something to point to
define issue and PR templates in your repo - Suggest a format to enforce consistency of PRs, which can reduce cognitive load - we use comments to give guidance - Use checklists to guide progress and establish requirements
I emphasize clear and direct because any squishy language will just be ignored. Example: checklist in screenshot!
1 - assign the PR to someone, and add reviewers... 2 - whether documented or enforced by convention or automation
during reviews Allows a person to commit the prescribed changes directly in the GitHub UI. Especially nice for very small changes. Becomes more difficult to manage when there are synchronized suggestions in multiple places, like changing a variable name.
the perceived importance is usually just tied to where you are in the power dynamic - this slide is the reason I started writing this talk - my first ever opinion on PRs & PR reviews - Is this feedback blocking or non-blocking? - are you expecting them to address this in this PR? another? - these are all things I want to know about your feedback so that I can respond correctly
a couple examples of formalizing this
conventional comments introduces a consistent structure to your review comments
As giving direction relates to how we expect others to act, just as important is radiating intent on how _we_ plan to act. ambiguity and uncertainty are what lead to those round trips which we know take a long time to resolve when working asynchronously. when we radiate intent on what we plan to do, we can snuff those out ... some specific things we can do in our PRs to radiate intent...
let them know your work is in progress (if you can remember that the Draft feature exists, which I never do) gives them a chance to see what you're thinking about this line of work
let them know this isn't to be merged ever there's a tendency to think every PR must be merged but opening a PR that proves out an idea is sometimes the best way to get good feedback because everyone can see the code, and pull it down to run it but when it's served its purpose, you can close it and re-implement with more production-worthy changes
what is part of this PR, and what will you cover in a follow-up PR?
on an epic level, what is included in this feature?
one aspect of pull requests that I think fully captures the problem of ambiguity and benefits tremendously from all the strategies we've talked about to remove ambiguity is the question of which changes should I be requesting in a PR review has anyone collaborated on a project where PR review feedback was imbalanced like one person wanted everyone else's code to look like they wrote it and others on the team were more like "ehhh I can understand it, it's good"? That's ambiguity at work.
identifying whether a particular change is fair to request is hard, because the answer is often "it depends" 1 - what's readable? if you're a functional programmer and I have a Java background and we're both writing javascript, we're going to have very different perspectives on which code is more readable 2 - how wrong is wrong? is it a variable name that doesn't make sense anymore? is it costing us money? where do we stand on the question of whether that fix should live in this PR or in a separate one? 3 - what kind of ownership does the project have? Is it a single owner/DRI/dictator? Is there no one owner but maintained by 5 different people?
All this to say Ambiguity leads to imbalances, and honestly inequities. It leads to people falling back on power dynamics to decide whether to request specific changes. That leads to different collaborators being held to different standards. That's usually not what you want. The best way to kill ambiguity is to define rules of engagement more precisely
section 4: tell stories
A good story has all of the qualities we've talked about so far But we can also talk about some more specifics
So that you can avoid the round-trips of clarifying questions, which in an asynchronous environment can drag things on and on and on One rule to live by ...
Even if the reader has perfect context when they read your message today In a month, when someone's looking back at this work, trying to figure out when, why, and how a feature was introduced... maybe that person is you... that context and knowledge won't be there. It's really helpful to have it all written up for posterity.
1 - what are we trying to do here? 2 - why does this new code exist? during a review, why do you recommend this change? what's going to happen in the system that they're not realizing? 3 - and why you chose this approach over the others and again remember that you could very easily create a PR template that encourages including these things
when possible, give them something visual so that they can see the effects without running the PR locally I don't expect you to be able to read anything on here, but I think you can see the difference in size of those orange squares
- you can give yourself a PR review! - this is often the first thing I do when I open a PR
and when a line is particularly tricky, that's a great time to explain
Though if you find yourself explaining a single line of code in a pull request review, there's a good chance you should also add that comment in the code doing a self-review when opening a PR is a good opportunity to catch things like this.
1 - in a PR, what's an example you followed? - in a review requesting changes, what's an example they can follow? 2 - (conversations, tickets, etc) - be aware of access & privacy
a good story is concise -- it doesn't drown you with superfluous info it's also cohesive -- the words that are there work together instead of against each other. In a pull request, we can easily fall into the trap of including everything you did since your previous one even if it's not directly related to the work we're doing And to avoid this, we want to make sure we break our work up into right-sized pieces Too big, or too sprawling, and they can be too difficult for a person to review, and they'll rubberstamp LGTM, and two weeks later we discover the bug we snuck into that big rubberstamped PR.
Sometimes it's obvious how to break up a PR that isn't concise or cohesive But often it's not my guiding light for separating the tricky ones is to break them up by mindset required to review them when a PR contains different types of work that require different mindets to review, it's more likely to draw the LGTM response, which translates to "there were too many things going on and I didn't totally get it all, but I inherently trust you." 1 - api vs front-end, etc 2 - infra invites more thorough review of concepts; - implementation is more "are you following the existing patterns." - includes the "walking skeleton" approach, where you start with a PR that just puts the infrastructure in place so that different parts _can_ function together...then implementing each of the functions into the skeleton. 3 - routine work that follows existing patterns usually gets "hey you missed this" type of feedback; novel work that takes thoughtful consideration of approaches invites more theoretical feedback. Ideally you can do this with separate PRs. If you can't, at the very least practice good commit hygiene to separate your work....which leads me....
One last potentially more controversial point on cohesive stories Commit hygiene
You can amend them along the way to meet these criteria
and when you do this, the commits _can_ tell a story and I agree that 95% of the time that doesn't matter, because we're just looking at the diff view of the PR anyway But sometimes it is easier to review a PR one commit at a time - especially refactoring PRs and It's nice to provide people that option (read for the back of room)
(read for the back of room) and as much as I love Pavlos and as hilarious as these commit messages are they don't provide me that option
waiting until the very end for feedback results in more re-work than collecting feedback along the way. 1 - use the draft feature if you remember it exists 😅 just make sure it's clearly marked as experimental/incomplete. - we'll talk more about this in a bit 2 - build prototypes that reviewers can play with by checking out the PR's branch 3 - build the infrastructure for a feature, and let people review that before you fill in the guts. 4 - these are technically better served as issues, but opening them can help you create a more successful PR. ... I give you this advice knowing full well that it is my own biggest struggle
I think the actual quote is "enemy of good" but it makes more sense to me as enemy of done. Again, this is as much a reminder to myself as it is to you, ... When it comes to PRs, perfection might be getting in the way of shipping things. and it leads to opening PRs only after the work is all done which leads to more re-work, due to waiting too long for feedback
section 6: Grow together
Seems kind of silly, given we're talking about collaboration But I think sometimes people forget just a couple small tips here
give credit where credit is due This is especially important when more experienced pair with less experienced If you work together on something, add both contributors to the commits. Give everyone credit so you're all building your portfolio of work.
whether it's to build shared understanding, or for mentoring purposes, or for the lottery factor, 1 - maybe it's something you learned about the product maybe it's something you learned about the programming language 2 - how did I do this thing that you might also need to do? what cool dev tool features did I learn about while testing this?