Blog

Pull Requests

Why does it sometimes takes a long time for a pull requests to be approved. To give you a better understanding this is the process I usually go through when performing a pull request.

Check to see if there is specific JIRA task associated with the pull request. If so read up on the pull request, see who created it, what comments are left, and see if there are any specific use cases mentioned. If the creator is on the pull request as a reviewer, I will see what they had to say, and give the final word to them. They know the issue best and should be the authoritative voice on the pull request. 

Step 1 : Code Review

Next, I will look at all the code, file by file, line by line and see what is changed. I will look at the following things: documentation, formatting, extra code, correctness and security.

Documentation: I will look and see if there is any documentation. Until Now, I have not really focused on documentation but will start doing that in the near future. I will check to make sure you have added documentation to your code, look at your algorithm and make sure that you have it documented. I will look at the code you added to the api section and make sure it has swagger documentation.

Formatting: I also will look at formatting of your code. Please try and stay close to the existing code formatting. Hopefully soon we will give you some hints on what we expect from your code. But make sure that if you add a new block of code, it is close to where the original code is. Make sure it is indented correctly. Make sure that you have spaces and no tabs in your code, and make sure that you don¹t add blank lines that are not needed. I will try and recognize any CR-LF issues if you checked in your code from a windows machine.

Extra Code: I will also at this point check to make sure you have no code that is commented out (and can be deleted), or debug statements that can be deleted. I will also try and see if you added new functions that are not used and can be removed. Next, I will look and check to make sure that it only deals with the specific issue you are solving, and make sure there is no additional code there that might confuse the specific pull request. Right now I have not done much, but I might ask you in the near future to split the pull request so it becomes easier to review. I will also look to see if you checked in any changes to play.plugins, or application.conf that  might break the code base for others, or potentially the deployment.

Correctness/Security: This is the hardest, but I will try and look at your code and identify any uses cases that might not be solved by your implementation, or any potential issues in your code. This is where some security checks are done. Do you write anything to the database that is not secure? Are your SQL statements safe? Are you writing text to the database that could result in cross site attacks, or allow for insertion of javascript code that will be rendered on the user page? Is the code that you have overly complex? Can we leverage some Scala functions to simplify the code? Should we use an Option[] instead of nulls?

Step 2 : Testing

At this point I have a pretty good understanding of what is done in the code. I know what is in the code, what has changed, and what I should test. Now I will start to check the actual functionality. I will check out the code on my local machine, try and make sure that I have all the pieces of code that are needed, and run the code. I will start by looking at the JIRA issue (or description of the pull request) to make sure it solves the actual problem. See if it actually works for some random cases. This is where  having looked at the code comes in handy. I can now try and find those cases that break your code. Next I will look again at what you have changed in the source code, and make sure I hit as many of the code changes as as possible.

Finally I will spend some time just randomly clicking on the app, trying to see if there are some unexpected consequences. I will also try and see what happens if I have the code not deployed at the root context, but what happens if it is at a different context (such as /medici).

If all of these issues are correct for me, I will hit the approve button. If I'm the last one I will merge and delete that branch. Congratulations.

As you can see there is quite a bit involved in a single pull request, if your change is only few lines (up to a 100 or so) and not in too many files, it is relatively easy for me to do the pull request and the whole process should take around half an hour. On the other hand, if you have many modifications, I will procrastinate as long as possible to start with that pull request. At this point I can spend a whole day on your pull request, if not longer. Every time I get interrupted, by a meeting, somebody asking me a question, I will need to spend some more time to get back to the pull request, and it takes longer to finish it.

TL;DR Keep your pull requests, short (200 lines or less) and to the point (1 JIRA task or 1 fix) and I will be able to check and approve your pull request quickly.

To be able to list files in tomcat6, need to edit web.xml

<init-param>
<param-name>listings</param-name>
<param-value>true</param-value>
</init-param>

To be able to list files in tomcat6, need to edit web.xml

<init-param>
<param-name>listings</param-name>
<param-value>true</param-value>
</init-param>