Tuesday, September 08, 2015

Enforcing Coding-Style checks on diffs


Have you seen how most of the code review [comment]s look like?

- This line exceeds 80 columns ...
- No space after ...
- No comma here ...
- Conditionals must have a block ...

Though these look insane, they ensure that code looks saner, when everyone follows the company's coding guidelines - unfortunately its not easy to enforce! Everyone has their own favorite editor and their own settings, add to this - there will be third-party code which has its own different style.

The best one can do is - when new code is added, can warn if the delta violates coding guidelines or not. Wait, delta? - diff ? How do we run checks on a diff!? Its not easy, but not too complicated either! at least for most common checks which the author should have figured out himself/herself before posting the code for review!

My approach: Simple Perl regex checks on diff hunks!

(If the mention of Perl + regex makes you feel nauseatic - stop here. :-))

Note: I tried this for C code diffs only.

We're not dealing with a function in its entirety, its just a diff, so how do we go about ?
of course, line-by-line :-/ duh!

  • Use unified diff, to get the context (file name, line range) 
  • Do some basic line merging logic when we are sure its not complete [1]
  • Check line by line, for basic style enforcement
    • length > 80?
    • trailing white-space ?
    • if/else not followed by a block ({) ?
    • etc...
  • Keep track of line numbers so that meaningful messages can be printed, pointing to the exact line where the issue is!

And how do we run this script automatically ?
Since I use git, git hooks! (just make it part of pre-commit hook)

Does it work well?
surprisingly well! :-)

[1] I used parentheses balance as a check to know if I need to merge lines or not.


--EDIT--

Do we need to do any extra work, than what's written above?
We do have to take care of  /* comments */ , and "strings"! regexes can be easily fooled, if we don't take enough care!

Can we make this fool-proof?
No! this can only be best-effort


No comments: