Last night's Perlmongers meeting was fun -- in September, we do Lightning Talks (not strictly limited to five minutes), which give everyone a chance to do a mini-presentation.

The discussion after Olaf Alder's presentation on Code::TidyAll touched on how to insure that developers only committed and checked in code that passed certain style rules. I'm curious to find out what my fellow monks feel about that.

At my last job, I was the git administrator, and I set up git hooks for some of the important repositories such that the commit message would be searched for a JIRA ticket number. If found, the script used the JIRA API to update that ticket with information about the commit, including a link to the commit (via gitweb). Theoretically, each commit should have had a JIRA ticket number, but if one wasn't found, the git hook script just output a message reminding the developer that their commit should have included an issue number.

I could have just disallowed the git push command altogether, forcing the developer game the system by using a fake JIRA number (ABC-1234) or perhaps some unrelated ticket number, but I wanted to avoid having the developers try to game the system in order to get their work into the repository. (I should also mention that the build system pulled only from the repository.)

What are your thoughts? How far should the revision control system go to enforcing a particular style and level of documentation?

Having unthinking robots block work progress can certainly be a very bad idea. I have two main things that I weigh when considering such things:

How obvious and mechanical is the test being performed?

How serious are the consequences of violations getting through?

To a lesser extent, I also consider how likely violations are to happen. But that has more to do with whether some kind of check should be added somewhere in the process and not whether an unthinking robot should be allowed to block work progress because of a detected violation.

Unless the test is darn near impossible to get wrong, I'm quite unlikely to approve of an unthinking robot blocking work progress based on the test. Even for dead-simple tests, I'm against blocking work progress unless the consequences are serious.

For example, consider checking in a source file that contains tabs or trailing whitespace. It often isn't rocket surgery to get that test written quite reliably (you have to exclude "binary" files, and where you draw that line might be easy and obvious or might be completely unclear, depending on the repo involved). But I still wouldn't have the resulting unthinking robot in a position to block work progress.

I can convincingly make the case that such whitespace violations are against our best practices, that there will never be a need for them in this repo, that it is easy to avoid them, and that these violations cause problems down the road. But the consequences of violations just aren't even close to serious enough to block work progress. They aren't even serious enough to agitate your team members by having an unthinking robot get in the way of them doing their job (even if the robot offers a way to work around the check).

I much prefer to have the feedback on tabs in source code to come from coworkers who have the capacity to think, even contemplate. And I set the example of not providing such feedback mechanically nor incessantly. We don't need coworkers resenting each other as acting like unthinking robots. So getting feedback about tabs doesn't lead to agitation or annoyance or aggravation.

Checking in source code containing "\r" characters can be quite different. Having a future merge be pretty much impossible to resolve because every single line on one side of the merge is considered to have changed because a "\r" got inserted on each can be a very serious consequence. (I have not seen a person check in source code where they converted all of the leading spaces to tabs on every line when the coding standard suggests the opposite -- and if I were to see such, I'd want to have a conversation with that person, not just have their check-in blocked or produce a warning.) So I have sometimes had unthinking robots scan for "\r" characters in source code before a check-in is accepted.

Style? Oh, hell no.

I guess I could see it if I were working in a Java shop where the parser that enforces (or changes) the style is guaranteed to not just get it wrong in some way, we are using Java because it makes it easy to replace workers with minimal work by Recruiting, because we just find it too hard to mentor workers so that they become more than code monkeys, we prefer code written so that even entry-level java programmers can easily understand each line, we have so much turn-over that we don't bother fostering a good team feeling, so outright bickering about style is just to be expected unless you bring in robots that are even more unthinking than your coworkers...

Yeah, you deploy the style-enforcing robots in that environment. I'll go check the job boards instead.

I prefer documented best practices that encourage people to figure out ways to improve the best practices. It took a long time to figure out an indentation for moderately complicated conditionals that actually works well. I'd never have bothered if some robot was mechanically enforcing the formatting in the mean time.

Then there is the option of having unthinking robots rewrite your programmers' code and then committing the results, unseen by the programmers. That sounds pretty scary to me when the code is Perl. But then I've seen perltidy interpret code wrong quite a bit (most people seem to think that never happens).

My criteria for unthinking robots modifying code with the results unreviewed by real programers are similar to my 2 criteria above for blocking work progress.

This also runs up against my usual surprise when people talk about syntax highlighting. I spend about as much time looking at code outside of my favorite editor as I do editing code. Much of that time is looking at diffs. I don't get the casual acceptance of editing a transformed version of code that isn't going to match what I see in all of the other places when I look at the code. I go to edit the code and the line numbers don't even match those from the error messages because the code being run is straight from the repo while my editor is showing it to me with my preferred (un)cuddling of braces and/or 'else's?

No thanks! I prefer to foster tolerance and learning from each other, even when it comes to matters of style.

Enforced style, especially where it is automatic, can be good. So I say. With such a system, everyone can have their own little style quirks in one's own editor that won't constantly show up as meaningless changes in commit diffs. The temptation to bikeshed indentation or brace placement, etc, goes away, and the risk with having your own cperl-mode or whatever also goes away. Mostly, kinda, sorta.

Documentation cannot be automated (well). I think a Pod coverage test that fails when subs/methods aren't documented (or license/synopsis/etc is missing/erroneous) is fine and I appreciate it because I forget to do that most of the time without a test.

I also think your previous practice of allowing "loose" commits was the best choice. Nagging is preferable to locked doors at the dev stage. Hard barriers make for unhappy work, and as you mentioned: gaming the rules. (Auto-)guides and peer pressure are better/friendlier for group standards.

The real problem with setting boundaries, AKA PROCESS, is that it almost guarantees sclerosis. Process gets less flexible and covers more ground over time, blocking out possibilities for improvement and innovation in work flow. So revisiting and being open to revising or trimming boundaries is something to consider and reconsider often.

In a previous career I had a boss who used to like to set hard-fast rules: "Under no circumstances should we ever ..." I would often grow uncomfortable at the proposed restrictions on how one must carry out the job, and would ask, "Never? How are we going to deal with ...?" Just about every time the reaction would be, "Well, that's a special case. How often does that happen?"

The point is that there are often more special cases than average moments in the day of a demanding job, and we really ought to be cultivating environments where people can excel by exercising their creative and hopefully talented minds in meeting those special challenges. Is it a good idea to take a moment to critically consider options before doing something that might disrupt the norm? Of course. But preventing one from disrupting the norm puts people to sleep, stifles creativity and productivity, and worst of all, prevents people from developing a sense of pride and ownership for their responsibilities.

So here's the hard fast rule I took from that repeated experience: "Under no circumstances should we ever ... make it impossible for people to adapt to unforeseen situations by thinking for themselves."

I think the cannonical example of policies enforced with good intentions but having negative side effects is enforcing password policies that make user's passphrases so hard to remember that they write them down and stick them with a post-it note to the side of their monitor.

Yes, develop clear guidelines for how the code should look ... indeed, for how the entire development process should be conducted by all concerned, from specification to quality-assurance to support. Involve every individual in establishing those procedures so that you can get both maximum “buy-in” and maximum professional input.

No, don’t use a computer as a gatekeeper or an Enforcer. (Computers only understand if .. then .. else, and no programming language really understands unless.) Instead, make that a team responsibility along with everything else. The computer is wonderful at “never overlooking a single detail,” and you should of course exploit that power wherever possible, but it cannot think, therefore it must never be put in a position in opposition to those who can.

The best “trick of the trade” is peer review, yet the primary benefit of this is not to enforce (arbitrary ...) “coding standards,” but rather, to catch mistakes. Mistakes in code-writing are rare. What isn’t rare?: mistakes in the understandings that led up to the design/writing of that code. Stuff that might easily pass one pair of eyes is very unlikely to make it past two pairs. People who are looking over each others’ shoulders are also talking. Cubicle walls can become silo walls, and ... careful, now! ... if you are both “a coder” and “a manager,” the very-worst offender just might be you! (Sure, you know the system inside and out, but does everyone else in your team know as much as you do? Uh huh ... go fix that.) If people are in agreement about “best practices” for your their shop, and if they are genuinely aware-of and involved in what their peers are doing alongside them, then high-quality code (and a high-quality total development environment) will be the result. In all my years, I have never met a “genuine slacker” who did not care about what s/he was dong, nor a “genuine incompetent.” Never. Software developers as a breed know how vital their work is, and strive to do it well. You just need to maintain an environment in which everyone including you can succeed.

Little if any at all. Educate your developers about what the shop-standards are and solicit their professional opinions of what it should be. Institute a system of peer-review and open communication. Computers can't "enforce" people doing what people ought to do, because computers are not smart as people are.