Posts categorized as coding

How to Unstick a Pull Request

Kerry Liu 10:35 am on April 11, 2021 → coding

Sometimes pull requests can get stuck during code review. In many cases, it’s not because the changes were unneeded, but because the conversation just appears to… well, stop.

I’ll walk through five common problems pull requests get trapped in and what you can do as a reviewer to help move things along.

The Forgotten Pull Request

Photo by cottonbro on Pexels.com

Symptoms: There might have been some conversation on the pull request but now it’s just dead silence. Reading back you see that the last comment was from six months ago without clear next steps. The pull request is still open, waiting for someone, anyone, to help nudge it.

Try: Ask the author if the pull request is still needed and if they’re interested in working on the problem. What happens next is either the pull request is swiftly closed OR the author tidies it up for a fresh set of eyes and a new review. ✨

What is the problem? What does this do?

Symptoms: The pull request summary is a terse sentence. In other cases, the pull request doesn’t summarize and has too much context. For whatever the reason, as a PR reviewer you’re not quite sure why the PR exists or how to test it.

Try: There’s no need to be a detective 🕵️‍♀️. If something is confusing, go ahead and ask the author! Being clear on why a PR is needed and what is does, is not only good for the reviewer, but it helps provide context to future maintainers on why certain decisions were made.

Ideally we want to know:

  • What problem(s) the PR is addressing.
  • What does the PR change?
  • How can we manually test it?
  • What type of feedback is the PR author looking for?
  • Are there any trade-offs to the solution we should be aware of?

PR Checks are failing

Symptoms: Newer contributors to a project might not realize that their pull request has failing checks. Maybe the linter is not happy with some whitespace, or a test is broken and the author isn’t sure on how to run the suite.

Try: Gently remind folks in a comment that checks have failed. If you have the time, point to documentation on how to run the tests and other contribution guidelines. It can also be helpful to troubleshoot environment issues with them or give them additional hints on how to clear up the issues. Using our best judgment, we can sometimes accept the PR in a less than perfect state and help fix the issues by asking and making changes on the branch directly, or in a follow up PR.

No Clear Next Steps

Photo by Leah Kelley on Pexels.com

Symptoms: We see a PR where other participants have left some previous reviews. It isn’t clear what needs to happen next. The conversation might even still be active, but it’s going on and on and on and on… to the point where GitHub thinks it’s a good idea to start hiding part of the timeline:

Try: Ask the active participants what needs to happen for the PR to either merge or be closed. It might be as simple as someone taking the time to summarize what was discussed and what needs to happen next, for example: fix tests, address design feedback, and update documentation. At other times, a PR working through multiple concerns might need to be split out: like starting a new PR exploring expanding an API, or proposing a framework change in a different medium like chat or long-form writing.

Additional Reviews Needed

Symptoms: You’re not confidant of approving the PR on your own. The changes affect multiple areas. Maybe the PR needs other expertise like design or security feedback that you’re unable to provide. You’ve done a great job already by knowing what you know and what you don’t know.

Try: Leave a review clearly stating what you tested, what feedback you have, and what type of feedback you think the PR needs to land. Manual testing and other partial reviews are still a great help to other reviewers. If you know who’d be great at unblocking the review, ping them in a comment with context. If you’re not sure, leave a note on what type of reviewer expertise is needed and if you have time, help the author by asking other contributors on who’d be a good fit to review.

Give it a try!

If you notice a stuck pull request that fits one of the patterns here, try unsticking it! You don’t need to be the project expert to help move things along. Simple actions like asking good questions, manually testing, or asking for additional review help can make all the difference.

#code #process

Code Reviews 📚

Kerry Liu 8:42 am on August 20, 2017 → coding

The following is a collection of my thoughts about what makes a good code review. This is a repost from the internal Calypso blog with a few modifications made from feedback. I have also included a few tips to structure PRs in a reviewer friendly way. It’s my hope that this post will help encourage folks to get excited about code reviews.

Frame of Mind

At the start of my career, I didn’t understand why good code reviews were helpful. This was partly because I hadn’t seen a good code review yet. At best someone might have rubber stamped my change, and at worst code gatekeepers nitpicked irrelevant details in the patch leaving both parties in a foul mood.

My thoughts on reviewing changed drastically once I realized I was approaching this with the wrong frame of mind. We are rewarded by what effort we put into it, and as part of that participants must share some common understandings to avoid an unproductive review.

At Automattic, I think we already have a very strong reviewing culture. The following are a few points I personally remind myself, before starting a review.

We all share ownership of the code. It is not yours, or mine, it’s ours. Always welcome improvements and share knowledge freely.

Many programing decisions are opinions. There is often no one right answer. Discuss tradeoffs in a productive way and move on quickly. Stick with project convention for stylistic things like tabs vs spaces, even if you don’t agree with it. Changing convention can be done outside of the PR as a larger discussion with the group.

We can always learn new things. No matter how much one knows, we can always learn more. Folks can expose you to new ideas. Explaining concepts you’re familiar with can help improve your understanding of it.

Communicating

Another huge part of a successful code review is good communication. We’re all nice folks at Automattic, but text communication is tricky. It is very easy to misinterpret feedback about code as something more personal. I think we’ve done a very good job at avoiding this, but here are a few techniques to lesson confusion.

  • Avoid separating code ownership. Do not assign ownership of the code with words like “my code” or “your code”. Doing so makes code reviews feel more like a personal judgement. We all share ownership of the code. Remember that the code is also a product of many constraints (time, familiarity with the codebase, etc.) and is not a personal reflection about the author. Even the best developers will produce code from time to time that has some issues to work through.
  • Assume best intent, stay positive. Avoid sarcasm and negative descriptors like “terrible” or “dumb” that may be misread.
  • Avoid demands, offer suggestions instead. “What about moving this into it’s own file?” It’s also helpful to phrase these as questions. Often times the reviewer may be missing context on why a particular suggestion will not work.
  • Authors should respond to suggestions. “Great catch! Updated in 565acae.” “We went ahead with the original approach because of timing concerns.”
  • Be explicit. “Let’s do change X because of reason Y”
  • Say if something is a blocker or optional. “Due to security concerns we should update this method before shipping.” “This is optional but I think this reads better if we move this into it’s own method.”
  • If something is confusing, ask. “What is the reasoning behind these changes?”  “I don’t understand what’s happening on this line, could you please explain?
  • Let the author know when you appreciate a change. “Thanks for taking on this task!” “I really ❤️ how this new workflow feels, I left a few notes on some things we can improve.” “This PR drops our build size by 500kb! Great work!”
  • Explain next steps, or complete the review. “I noted a few blockers I’d like to see resolved before we 🚢” “Changes here look great. 🚢 when you’re ready.”
  • Keep up momentum. If a PR looks stalled ask if anything needs to be done. This is especially important for OSS contributors. It is usually better to accept a PR that has a few issues left to work through, and fix it up later, than have the OSS person abandon the PR.

Preparing your PR to be Reviewed

If folks are always waiting for a code review it helps to have some empathy for the reviewer too!

  • Explain why. Assume reviewers have little or no context when reading the PR. Explain why we need this PR and what it does. (This is also very useful when looking at past decisions). Screenshots and gifs are appreciated when behavior is complex.
  • Add Step-By-Step Test Instructions. Can someone unfamiliar with the changes test your PR by reading the summary?
  • Keep changes small. Large changes are difficult to review and understand. Try to separate janitorial changes from PRs that change behavior.
  • Note weird things. This includes explaining any odd code workarounds, or buggy behavior. This can save some back and forth between the reviewer and author, and may also expose existing bugs.

Code Review Benefits

When done well, code reviews can help on many different levels.

  • It spreads code ownership.
  • Communicates changes across teams.
  • Serves as a sanity check to verify that requirements make sense.
  • Allows folks to find bugs though both manual testing and in code.
  • Lets all folks involved learn new things!
  • Can also serve as documentation for how something worked, and why certain decisions were made. Perhaps even for a future you!

Anyone can be a Reviewer!

The fact that code reviews work on many levels also means that reviewers don’t need to know all things about a project in order to make a meaningful contribution. Sharpening copy, manually testing, polishing design, or asking questions about confusing things is a great help.

Code Review Challenge

If this isn’t a habit for you yet, I’ll like to challenge you to try reviewing a few PRs from a different team or one that you may have felt intimidated to contribute to.

Here are a few strategies I use to pick PRs to review:

  1. Take a look at one of the oldest PRs on the needs review list. Ping the author with questions if it looks inactive.
  2. If you don’t have a lot of time, choose a tiny PR to look at. These are the fastest to review and test, and usually have the least risk of causing a regression.
  3. Choose something you’re unfamiliar with. Reviewing PRs is a great way to learn, and to keep a pulse of what’s happening on other teams. Don’t be afraid to dive into a section you’ve never looked at before. If you don’t follow, or something is unclear, ask questions! The PR author is usually happy to explain.

Have fun reviewing!

#code #process

Why didn’t that selector work?

Kerry Liu 11:52 am on September 10, 2015 → coding, css

CSS Specificity 

Ever get frustrated when a newly added CSS rule just doesn’t work? CSS specificity is how a browser determines which CSS properties will be applied to an element. If this is the first you’ve heard about this, don’t worry, a surprising number of engineers I meet don’t understand how CSS specificity works and can probably help explain why CSS naming conventions like BEM are so popular. (If you follow BEM, your score should typically never go past 0,1,0 or 0,2,0).

In a nutshell:

  1. The selector with the higher specificity score wins
  2. For selectors with the same specificity score, the last one wins (rule closest to the bottom of the stylesheet)
  3. Inline styles added to an element element overwrites any styles in the CSS
  4. !important will override all other specificity.
  5. If you have two matching !important rules, the last one wins.

How do you calculate specificity?

It’s actually pretty simple. CSS specificity is just counting. If you look at the specification:

A selector’s specificity is calculated as follows:

  • count the number of ID selectors in the selector (= a)
  • count the number of class selectors, attributes selectors, and pseudo-classes in the selector (= b)
  • count the number of type selectors and pseudo-elements in the selector (= c)
  • ignore the universal selector
Selectors inside the negation pseudo-class are counted like any other, but the negation itself does not count as a pseudo-class.

Concatenating the three numbers a-b-c (in a number system with a large base) gives the specificity.

*               /* a=0 b=0 c=0 */
LI              /* a=0 b=0 c=1 */
UL LI           /* a=0 b=0 c=2 */
UL OL+LI        /* a=0 b=0 c=3 */
H1 + *[REL=up]  /* a=0 b=1 c=1 */
UL OL LI.red    /* a=0 b=1 c=3 */
LI.red.level    /* a=0 b=2 c=1 */
#x34y           /* a=1 b=0 c=0 */
#s12:not(FOO)   /* a=1 b=0 c=1 */
.foo :is(.bar, #baz)
                /* a=1 b=1 c=0 */

Lets break that down a little bit further.

Concatenating the three numbers a-b-c (in a number system with a large base) gives the specificity.

ID Selectors > Class Selectors > Type Selectors

Here’s the tricky part. No matter how many type selectors you have, it’ll never trump a single class selector, and similarly no matter how many class selectors you have, it’ll never trump a single ID selector. Instead of using base-10, you can put a comma between each of the selector types.

So for example the top .foo selector cannot be overridden by a selector which only uses type selectors, no matter how many of them you add to your selector. For convenience, most folks will use base-10 since hopefully your CSS file doesn’t have selectors like this.

//specificity=0,1,0
.foo
//specificity=0,0,11
html body div ul li span span span span span span

What about inline styles?

Inline styles always overwrite any styles in the CSS. So the div in this case will be red, and this is true no matter how many id selectors we add to the css rule.

<div id="green" style="background:red;"></div>

#green {background: green;}


!important

In this case, our div actually will be green, despite the inline selector, or any CSS rules. It’s common knowledge to avoid using !important when you can use alternative means, because you’ll likely waste a lot of time for your future-self or your co-workers when you can’t override that important rule you just added.

<div id="green" style="background:red;"></div>

#green {background: green !important;}


When using !important is OK

That being said, !important is a tool and there are times when it’s acceptable to use it.

  1. Overriding Media Query Styles – Need to override a rule for a particular target screen-size? You can avoid this in some cases, but if you use a CSS framework like Bootstrap, you already have these rules in place.
  2. Utility Classes – Want that .pull-right to really stick to the right? !important. It’s easily arguable that you don’t need utility classes at all, but this is how you get them to work.
  3. Email Styling – Email styling is like stepping back in time. You use tables everywhere, clients have terrible CSS support, and sometimes the only way to override default client styling is by using !important.

#css #specificity