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:
Post a Comment