SVN Commit Hooks For a Better Codebase
This is a cross post from the Wayfair Engineering Blog
As we have mentioned before, the main source control system we use at Wayfair is SVN, with TortoiseSVN as our client. One of the things we love about SVN is the ability to add commit hooks, or checks that run when someone tries to commit a file to source control. By having a few key checks we can prevent bugs, ensure consistent coding practices, and generally have a cleaner codebase.
The first commit hook we added was a PHP lint check. This means that when you try to commit a PHP file we run php -l on it from the command line, ensuring that the file has no syntax errors:
This might seem like a paranoid check, and you might assume that this kind of thing never happens, but with our ASP code (where we have no syntax check) we saw people committing files with invalid syntax on a semi-regular basis. Sometimes people are making the same change in a number of files and fail to test each one, or sometimes mistakes are made when people are rushing. This commit hook is a quick and easy way to prevent a silly (but extremely dangerous) problem.
The second big hook that we have is a coding standards checker. Our ASP codebase was a huge mess, with tons of different styles of programming evident throughout the code. Dozens of engineers had worked on the code, and you could tell. We wanted a way to make things more consistent from a code layout perspective in PHP (I for one HATE incorrect indentation). We needed a way to not only establish a set of rules for people to follow, but also a way to enforce these rules. Luckily someone has already done this for PHP, so we were able to just implement an existing solution (we love the PHP community). There are actually a couple of options for this kind of thing, but we settled on the PEAR Coding Standards, with some modifications. Since the checks are written in PHP, it was easy to modify the default set of rules. We commented some out (we allow short tags for example), modified others (we bumped the line length limit to 200 characters), and used the majority of them as is. The other great thing about using an existing solution is that the documentation is already written for us. If you try to commit a file that violates any of these checks your commit will fail:
Developers can also run this check on demand on their dev machines, so they don’t have to wait until they are ready to commit to find out about any problems with their code. At first this change was met with resistance, and we lost some efficiency because people were having to correct a lot of “style” errors when they tried to commit. That was short lived though, and now that this has been in place for a while people are used to the rules. Our engineers almost never encounter these errors now, because their coding style has adjusted to fit the requirements of the check. This gives our codebase a consistent look, so when you jump into a piece of code for the first time you don’t need to spend 15 minutes just trying to parse what someone else wrote.
We are writing a couple of additional checks right now, like making sure that no one puts dev paths in CSS files and checking for SVN conflict text in a file that is being committed (the text that says “<<<<<<< .mine” when you have a conflicted file). We also have plans to run a CSS Lint check when someone tries to commit CSS files, failing on errors only. The goal of all of these checks is to prevent bugs first and foremost, but also to reduce technical debt and make our codebase more fun to work with. So far this has been extremely successful, and it’s only going to get better!
As we have mentioned before, the main source control system we use at Wayfair is SVN, with TortoiseSVN as our client. One of the things we love about SVN is the ability to add commit hooks, or checks that run when someone tries to commit a file to source control. By having a few key checks we can prevent bugs, ensure consistent coding practices, and generally have a cleaner codebase.
The first commit hook we added was a PHP lint check. This means that when you try to commit a PHP file we run php -l on it from the command line, ensuring that the file has no syntax errors:
Rejected commit due to a syntax error in a PHP file |
This might seem like a paranoid check, and you might assume that this kind of thing never happens, but with our ASP code (where we have no syntax check) we saw people committing files with invalid syntax on a semi-regular basis. Sometimes people are making the same change in a number of files and fail to test each one, or sometimes mistakes are made when people are rushing. This commit hook is a quick and easy way to prevent a silly (but extremely dangerous) problem.
The second big hook that we have is a coding standards checker. Our ASP codebase was a huge mess, with tons of different styles of programming evident throughout the code. Dozens of engineers had worked on the code, and you could tell. We wanted a way to make things more consistent from a code layout perspective in PHP (I for one HATE incorrect indentation). We needed a way to not only establish a set of rules for people to follow, but also a way to enforce these rules. Luckily someone has already done this for PHP, so we were able to just implement an existing solution (we love the PHP community). There are actually a couple of options for this kind of thing, but we settled on the PEAR Coding Standards, with some modifications. Since the checks are written in PHP, it was easy to modify the default set of rules. We commented some out (we allow short tags for example), modified others (we bumped the line length limit to 200 characters), and used the majority of them as is. The other great thing about using an existing solution is that the documentation is already written for us. If you try to commit a file that violates any of these checks your commit will fail:
This commit failed because the PHP didn't pass our coding standards check |
Developers can also run this check on demand on their dev machines, so they don’t have to wait until they are ready to commit to find out about any problems with their code. At first this change was met with resistance, and we lost some efficiency because people were having to correct a lot of “style” errors when they tried to commit. That was short lived though, and now that this has been in place for a while people are used to the rules. Our engineers almost never encounter these errors now, because their coding style has adjusted to fit the requirements of the check. This gives our codebase a consistent look, so when you jump into a piece of code for the first time you don’t need to spend 15 minutes just trying to parse what someone else wrote.
We are writing a couple of additional checks right now, like making sure that no one puts dev paths in CSS files and checking for SVN conflict text in a file that is being committed (the text that says “<<<<<<< .mine” when you have a conflicted file). We also have plans to run a CSS Lint check when someone tries to commit CSS files, failing on errors only. The goal of all of these checks is to prevent bugs first and foremost, but also to reduce technical debt and make our codebase more fun to work with. So far this has been extremely successful, and it’s only going to get better!
Comments
Post a Comment