On code formatting (and testing)

[Andrew Bartlett]

On Mon, 2017-04-03 at 09:18 -0700, Jeremy Allison via samba-technical
wrote:
> On Mon, Apr 03, 2017 at 04:48:56PM +1200, Andrew Bartlett wrote:
> > On Fri, 2017-03-31 at 15:05 -0700, Jeremy Allison via samba-
> > technical
> > wrote:
> > 
> > I'm sorry you feel this way.
> > 
> > In terms of my code formatting, I work strongly for clarity and
> > consistency.  If look at this file, you will see all the debug
> > messages
> > are formatted that way.  
> > 
> > Naturally I've adapted that line (only) in the updated patches I
> > sent
> > metze today. 
> > 
> > I totally agree that having a globally consistent style in Samba
> > would
> > be a good thing, and we actually do pretty well, which is why I
> > find
> > outbursts like this quite as frustrating as you find what you feel
> > is
> > deliberate non-conformance.  
> > 
> > It isn't that I'm out to 'break the rules', it is actually that I
> > just
> > code primarily to match what is already present, secondly to
> > whatever
> > is the clearest expression for the task.  Coding to our standard
> > style
> > comes naturally when the rest of the file already conforms, and the
> > problem space permits, but when it doesn't, or was written by
> > others
> > who didn't take them as strictly as you do, we end up in a pickle
> > like
> > this.
> > 
> > In short, all things in moderation, even 80 column limits. :-)
> 
> That's where we disagree. I don't want to keep existing
> style in existing files when updates are done. One of the
> good reasons for changing to >80 columns is it become a
> visual signature in a file if there are 80+ column lines,
> and <80+ column lines is that the code here has been
> updated. No it isn't good enough to track what was done
> (git blame rules :-) but it's enough to raise awareness.
> 
> I know you think this is a silly rule, but details
> *matter*, and enough of the rest of the Team members
> adhere to it that when someone doesn't it really stands
> out. We can always vote on it being a silly rule and
> agree to drop it if that is the majority opinion.
> 
> But right now we should *always* code to our standard
> style in new code. That way, eventually the whole project
> will get upgraded piece by piece :-).

No, that way we end up with a dog's breakfast. 

New files, substantial rewrites, fine!  Improving totally unreadable
code prior to starting, reasonable!  Mixing old and new style line-by-
line, really?

That is why I've always opposed rules 'for new code' that don't come
with patches making at least a majority of our existing code comply. 

We have seen how to do that well with the work Garming did early in his
time to expand some of the hidden return macros. 

Now, there is something I do feel as passionate about, and that is the
amount of 'obviously correct' code that lands without tests.  We have a
great record in Samba for automated testing, but I do wish we had as
much zeal for comprehensive unit and integration tests as we do for
patch-perfecting.  

All things in moderation?

Thanks,

Andrew Bartlett

-- 
Andrew Bartlett                       http://samba.org/~abartlet/
Authentication Developer, Samba Team  http://samba.org
Samba Developer, Catalyst IT          http://catalyst.net.nz/services/samba