Code reviews are a must, but can be quite a pain, right? Some of it can be automated by using Danger & klint so that you can focus on meaningful and constructive development only, not typos and such. Guest post by Joe Birch.
Hi, my name's Joe. I’m an Android Engineer and Google Developer Expert for Android, Flutter and Google Pay based in Brighton, UK working on the Android team at Buffer. I’m passionate about coding and love creating robust, polished and exciting projects for mobile, the web, TV, wearables and I’ll probably be toying with whatever the new thing is at the time you’re reading this – I love to be constantly learning. I’m also a keen writer as I love to share my learnings and experiences with others. (Twitter, Medium)
There are many benefits that come from code reviews - they are an important part of the development process. Whilst there are many things we can achieve from this process, they often help us to catch any issues before the code reaches our users, maintain a strong codebase, build on existing ideas and most importantly learn from one another. However, with code reviews come common code review tasks which can often result in a lot of comments from the reviewer - these common checks for styling, lint errors or test absence can often increase the review time which often leads to the clouding of visibility on more important issues such as bugs or architectural decisions. As well as this, if there are a lot of stylistic or test focused comments then this can in some cases make the author of the Pull Request feel under pressure from so many comments coming directly from colleagues - whilst constructive code review comments are awesome, anything that can come from a neutral source can reduce any of these feelings when it comes to code reviews.
Whilst that’s all well and good, what can we do about it? That’s where a small collection tools can come together to automate some common code review tasks so that we can focus on reviewing a smaller subset of PR content whilst letting the machines take care of the rest. For this we’ll mainly be using Danger & ktlint to help us achieve this in Bitrise (for different CIs the steps will likely be very similar). When we put the use of these tools together, we can get something like this on our Pull Requests:
Whilst at a first glance this may just seem like “simple” checks, having this in place can prove to be a pretty powerful addition to your code process. Whether it’s styling, potential bugs or missing tests - anything that you can fix up before passing your Pull Request on to be reviewed by a human can really help to smoothen the code review process.
With that in mind, let’s start taking a look at how all of this comes together!
Note: In this post I’m not going to cover the actual setup of Danger itself. If you need to setup Danger then you can do so here.
When it comes to this automated code review process there are several different steps that need to take place before we can run Danger, the tool that will actually perform this commenting for us.
We need to begin by running ktlint, this will generate the required “errors” that are generated when our code is run against the rules that ktlint defines. By default ktlint rules are based off of the standard kotlin code styling and once these errors are collected, reports will be generated for Project code (main directory), Unit Test code and UI Test code. These reports will e placed at:
${project.rootDir}/modulename/build/reports/ktlint/reportfile_name.xml- Next we need to run Android Lint, ktlint at this point is only handling styling errors - so we want to use Android Lint so we can pick up any Android errors or warnings from the code that we’re running our checks against. There will only be a single report file per-Android module, which will be located at:
${project.rootDir}/module_name/build/reports/lint-results.xml- We then need to merge all of the ktlint reports together into a single file - this is because our Danger configuration only takes a single checkstyle report. We can do this within our CI though to keep our Android Project code a little cleaner.
- Now that our ktlint reports have been merged, we need to go ahead and do the same for our Android Lint reports - this is again because our Danger configuration will also only take a single lint report. Again, this merging is done on our CI.
- Within Bitrise we then need to copy these generated reports over to our deploy path. This is because our Danger configuration file lives within our Android Project, so we want our reports to be accessible to this when Danger is run. Here we simply use Bitrise declared variables to copy our generated reports to.
- Finally we need to run Danger. This is done by a single command from within a script step.
Now before we can get started with any of this, we need to setup our Android Project for it to have support for ktlint and Danger. We’re going to begin by setting up ktlint, for this we’re going to use a ktlint plugin that makes the setup and running of this slightly easier for us.
In our project build.gradle we’re going to add the declaration for the plugin:
classpath "gradle.plugin.org.jlleitschuh.gradle:ktlint-gradle:3.0.0"And then make it accessible to all of the sub-modules within our project. Here we also configure ktlint with some general configuration settings:
subprojects {
apply plugin: "org.jlleitschuh.gradle.ktlint"
ktlint {
version = "0.15.0"
android = true
reporter = "checkstyle"
ignoreFailures = true
}
}Once synced, ktlint will be accessible to all of our modules. You can confirm this by running ktlintCheck for a specific module, such as:
./gradlew :your_module:ktlintCheckNow if you head on over to the build/reports/ktlint directory of the module that you run ktlint in you will see the generated reports. These reports alone aren’t everything that we want though, we also need to run our Android Lint checks. So that we can keep our calls here in a grouped space, we’re going to define a new gradle task to house both of these operations:
task runChecksForDanger() {
group = "Reporting"
dependsOn 'ktlintCheck'
dependsOn ':mobile-ui:lint'
dependsOn ':cache:lint'
dependsOn ':presentation:lint'
def file = file("${project.rootDir}/build/reports/ktlint")
if (!file.exists()) file.mkdirs()
def lintFile = new File("${project.rootDir}/build/reports/lint")
if (!lintFile.exists()) lintFile.mkdirs()
}The last part of this task creates the required files in our project so that bitrise can write to these reports, which will then be used by the Danger process.
Now you should be able to run ./gradlew runChecksForDanger from the root of your project and you’ll notice that both ktlint reports and android lint reports are generated in their corresponding directories.
The last part of our Android project that we need to configure is Danger. To get setup with Danger you can follow the setup guide they have written here. Once you’ve followed this setup we’ll need to go and add two plugin declarations to our Gemfile:
gem "danger-checkstyle_format"
gem 'danger-android_lint'The first one, danger-checkstyle_format, will allow us to provide a checkstyle report for danger (from ktlint) and the second will allow us to provide the report generated by the Android lint check. Once you’ve installed these plugins, we need to configure them within our Dangerfile:
# ktlint
checkstyle_format.base_path = Dir.pwd
checkstyle_format.report "/bitrise/src/build/reports/ktlint/ktlint-report.xml"
# AndroidLint
android_lint.report_file = "/bitrise/src/build/reports/lint/lint-report.xml"
android_lint.skip_gradle_task = true
android_lint.severity = "Error"
android_lint.lint(inline_mode: true)The most important parts here are the report and report_file directories - these are the paths to the reports that we wish to be used by the plugins / danger.
Note: If you want to only have messages posted within the diff of the current PR, be sure to add this to your Dangerfile:
github.dismiss_out_of_range_messages
Now that we’ve defined everything that needs to happen from within our Android project, we’re ready to start the configuration on the Bitrise side of things. For this we’re going to create 5 different workflow steps. We could have these separately, but the separation helps to keep things tidy and easier to follow.
We’re going to begin with the first step, which we’ve called Run ktlint. This step is simply going to be used to install the required dependencies for Danger, and then run our custom gradle task that we defined:
#!/bin/bash
set -ex
bundle install
./gradlew runKtlint
Now that we’ve run ktlint, our required reports would have been generated and now be available from their corresponding directories. the next thing we need to do is create a task to merge the ktlint reports together, we’ll handle Android lint reports in a separate script.
Let’s start off with the script to merge our Ktlint reports together:
import sys
import os.path
from xml.etree import ElementTree
first = None
file_list = [
"/bitrise/src/cache/build/reports/ktlint/ktlint-debug.xml",
"/bitrise/src/cache/build/reports/ktlint/ktlint-debugUnitTest.xml",
"/bitrise/src/remote/build/reports/ktlint/ktlint-main.xml",
"/bitrise/src/remote/build/reports/ktlint/ktlint-test.xml",
"/bitrise/src/base/build/reports/ktlint/ktlint-debug.xml",
"/bitrise/src/base/build/reports/ktlint/ktlint-debugUnitTest.xml",
"/bitrise/src/base/build/reports/ktlint/ktlint-debugAndroidTest.xml"]
ktlintFile = 'ktlint-report-orig.xml'
editedKtlintFile = 'ktlint-report.xml'
for filename in file_list:
if os.path.isfile(filename):
data = ElementTree.parse(filename).getroot()
if first is None:
first = data
else:
first.extend(data)
if first is not None:
f = open( ktlintFile, 'w' )
f.write( "<?xml version=\"1.0\" encoding=\"utf-8\"?>\n" )
f.write( ElementTree.tostring(first) )
f.close()
delete_list = ["/bitrise/src/"]
fin = open(ktlintFile)
fout = open(editedKtlintFile, "w+")
for line in fin:
for word in delete_list:
line = line.replace(word, "")
fout.write(line)
fin.close()
fout.close()Let’s take a run through this:
- We begin by taking all of the reports that we would have generated from our previous task:
file_list = [
"/bitrise/src/cache/build/reports/ktlint/ktlint-debug.xml",
"/bitrise/src/cache/build/reports/ktlint/ktlint-debugUnitTest.xml",
"/bitrise/src/remote/build/reports/ktlint/ktlint-main.xml",
"/bitrise/src/remote/build/reports/ktlint/ktlint-test.xml",
"/bitrise/src/base/build/reports/ktlint/ktlint-debug.xml",
"/bitrise/src/base/build/reports/ktlint/ktlint-debugUnitTest.xml",
"/bitrise/src/base/build/reports/ktlint/ktlint-debugAndroidTest.xml"]- We then declare the references to two files. ktlintFile is
ktlintFile = 'ktlint-report-orig.xml'
editedKtlintFile = 'ktlint-report.xml'- In the next part we read the contents of our file and write them to a file for each file in the declared list. In the second step here you will notice that we are writing an XML header to the report - this is because it is required for the report to be read correctly (it won’t work without this).
for filename in file_list:
if os.path.isfile(filename):
data = ElementTree.parse(filename).getroot()
if first is None:
first = data
else:
first.extend(data)
if first is not None:
f = open( ktlintFile, 'w' )
f.write( "<?xml version=\"1.0\" encoding=\"utf-8\"?>\n" )
f.write( ElementTree.tostring(first) )
f.close()- Finally, depending on your CI you may need to remove some path identifiers. I wanted to keep this part of the script separate incase you don’t need it. On bitrise, you will need to remove the bitrise deploy path from the file paths that are state in your reports. Otherwise Danger won’t be able to write the inline comments, as the paths in the report won’t match the paths in the project. So here we simply loop through the file and remove any instances of "/bitrise/src/" that occur. Note: If you do need this piece of code then you can combine it with the above for efficiency.
delete_list = ["/bitrise/src/"]
fin = open(ktlintFile)
fout = open(editedKtlintFile, "w+")
for line in fin:
for word in delete_list:
line = line.replace(word, "")
fout.write(line)
fin.close()
fout.close()Next we need to merge all of our Android Lint reports - this process is pretty much identical to the previous, expect the file list is slightly different. I show this separately here so that you can have this responsibility inside of a separate script step - however you may wish to place them inside of the same step and reuse the writing functionality so that you are not duplicating code.
import sys
import os.path
from xml.etree import ElementTree
first = None
file_list = ["/bitrise/src/cache/build/reports/lintresults.xml",
"/bitrise/src/presentation/build/reports/lint-results.xml",
"/bitrise/src/mobile-ui/build/reports/lint-results.xml"]
lintFile = 'lint-report-orig.xml'
editedlintFile = 'lint-report.xml'
for filename in file_list:
if os.path.isfile(filename):
data = ElementTree.parse(filename).getroot()
if first is None:
first = data
else:
first.extend(data)
if first is not None:
f = open( lintFile, 'w' )
f.write( "<?xml version=\"1.0\" encoding=\"utf-8\"?>\n" )
f.write( ElementTree.tostring(first) )
f.close()
delete_list = ["/bitrise/src/"]
fin = open(lintFile)
fout = open(editedlintFile, "w+")
for line in fin:
for word in delete_list:
line = line.replace(word, "")
fout.write(line)
fin.close()
fout.close()The next task that we need to carry out is the copying of our reports to the bitrise deploy path so that they are accessible by our project when danger is executed. Let’s take a look at this code:
#!/usr/bin/env bash
# fail if any commands fails
set -e
# debug log
set -x
# write your script here
cp ktlint-report.xml $BITRISE_DEPLOY_DIR/ktlint-report.xml
cp ktlint-report.xml $BITRISE_SOURCE_DIR/build/reports/ktlint/ktlint-report.xml
cp lint-report.xml $BITRISE_DEPLOY_DIR/lint-report.xml
cp lint-report.xml $BITRISE_SOURCE_DIR/build/reports/lint/lint-report.xml
less /bitrise/src/build/reports/ktlint/ktlint-report.xml
less /bitrise/src/build/reports/lint/lint-report.xml- We first begin by copying the ktlint reports to the bitrise deploy path. Here the cp command is used to copy the declared file to the specified location. This places our ktlint-report.xml file in our project directory so that Danger can read it when it is executed
cp ktlint-report.xml $BITRISE_DEPLOY_DIR/ktlint-report.xml
cp ktlint-report.xml $BITRISE_SOURCE_DIR/build/reports/ktlint/ktlint-report.xml- We then do the same for our generated lint reports.
cp lint-report.xml $BITRISE_DEPLOY_DIR/lint-report.xml
cp lint-report.xml $BITRISE_SOURCE_DIR/build/reports/lint/lint-report.xmlThe less commands above are used for debugging purposes - at some point you may need to see the contents of your files, so this is a good place to do so.
The final script step in our process is the actual running of the danger command. Here we just execute danger and the tool will take care of the rest for us:
bundle exec dangerNow that we’ve setup our scripts to run for Danger, we need to configure two plugins in our Danger file so that these reports will be read. For this you need to setup:
- Checkstyle format
gem install danger-checkstyle_format- Android Lint
gem install danger-android_lintThere is more information on these over at the Danger plugin information section, but we essentially just need to provide the paths to the reports that we previously generated along with c couple of configuration details:
# ktlint
checkstyle_format.base_path = Dir.pwd
checkstyle_format.report "/bitrise/src/build/reports/ktlint/ktlint-report.xml"
# AndroidLint
android_lint.report_file = "/bitrise/src/build/reports/lint/lint-report.xml"
android_lint.skip_gradle_task = true
android_lint.severity = "Error"
android_lint.lint(inline_mode: true)Phew! That wasn’t too much was it! During this process the plugins that we have setup will use our generated reports to print out messages onto our pull requests. When you eventually run danger as part of your CI workflow, you should see something similar to what we looked at above:
There are a few extra things that you can place within your project Dangerfile for further commenting on your pull requests, let’s take a look at a few of the things you could do:
- Give a warning if the PR title doesn’t match some expected state:
warn("I noticed this is marked as Work In Progress, does it need to be open before it is ready? 🙂") if github.pr_title.include? "[WIP]"
- Give a warning if the PR description is empty:
warn("You should provide a summary in the Pull Request description so that the reviewer has more context on this Pull Request 🤔") if github.pr_body.length < 5
- Give a warning when a PR is over an expected size:
warn("This PR is quite a big one! Maybe try splitting this into separate tasks next time 🙂") if git.lines_of_code > 600
Thank the author for their contributions:
message "Thank you so much for your work here @#{github.pr_author} 🎉 You might find a few suggestions from me for this Pull Request below 🙂"
As well as these there are other things you could do, such as checking whether a file has been changed that shouldn’t have been, or even checking if a file has been changed and it’s corresponding test hasn’t been. Once you get comfortable with Danger you’ll find that there is plenty you can do to ease your code review process - this allows you to make time for the checking of things that really matter and reduce any friction that may arise from these common code review aspects.
I hope this article has proved to be of some use when it comes to configuring your Bitrise projects with Danger. If you have any questions, or need any help setting this up then please feel free to leave a response below
.jpg)
.jpg)
