At today’s internal JT tech talk we had some tangential remarks on syntax highlighting in Github diffs. It was a snazzy presentation on Swift by Daniel Garcia and Luis Recuenco and the former was pointing out that it was sometimes difficult to tell apart base classes from traits just looking at the code.
Today as part of our #jttechtalks, @fillito and @luisrecuenco introducing @SwiftLang ! pic.twitter.com/12dRZM4R0u
— JT Engineering (@jobandtalentEng) January 19, 2016Immediately, the slick audience asked whether the community had settled with some naming convention to sort this issue out. Almost as automatically, I saw the parallel with the old practice of using a prefix for member variables in C++/Java and the remarks of Uncle Bob in Clean Code1:
You also don’t need to prefix member variables with
m_anymore. […] And you should be using an editing environment that highlights or colorizes members to make them distinct.
So I proposed exactly that: “Why not opening a feature request on whatever-IDE-you-use issue tracker asking for such highlighting?” “What about Git diffs?”, another colleague replied.
Like any standard Simpsons chapter, that brings us to the actual point of this post: to what extent you can review PRs by just inspecting the diff? (No matter how well highlighted).
For small or simple changes the diff should be enough, even more after Github implemented diff context expansion.
Image from Github's blog
However, by reviewing plain diffs without the ease of navigation and other goodies that we enjoy when having the code locally we risk doing shallow reviews. It is pretty sad reporting a bunch of style nits while we miss some architectural problem.
I download the changes when they are complex and/or I suspect a package has grown too much or weird module dependencies2. Do you do the same?