Coding style and whitespace

While I was doing a merge of recent Debian and Ubuntu changes I found
a conflict caused by a reforrmatting of the patch I submitted in
#390916:

In my supplied patch I wrote:
case dep_depends: fmt= _("%s depends on %s"); break;
case dep_predepends: fmt= _("%s pre-depends on %s"); break;
case dep_recommends: fmt= _("%s recommends %s"); break;

However in dpkg 1.14.4 I see:
case dep_depends:
fmt = _("%s depends on %s");
break;
case dep_predepends:
fmt = _("%s pre-depends on %s");
break;
case dep_recommends:
fmt = _("%s recommends %s");
break;

There are two formatting differences here:
* Tabular formatting versus vertically-laid-out formatting;
* Space before `='.

I can see no excuse for the formatting actually applied on any of the
conventionally applied criteria:

* When making changes one should generally adopt the style of the
original code. (The original code uses tabular formatting of switch
statements where possible and does not place a space before
assignment `='.)

* One should not change formatting without a good reason. I supplied
a patch but the formatting actually applied was different. This
has caused me needless work during my merge.

* The formatting should be clear and easy to read.

It is plain that the tabular formatting is easier to read. You can
trivially see that the code has a perfectly regular structure and
simply read off in each case what the details are. We use one
visual dimension for the different cases and the other for the
different details and furniture required for each case.

In the vertically-separated case it is much more difficult to check
that all of the cases are in fact identical and the boundaries
between cases are marked only by and indent and no longer by the
rows of the tabular structure.

The case of the space before `=' is less clear on this point but I
prefer the missing space beforehand so as to emphasise the
asymmetric nature of the operator.

* If reformatting, one should do so in a way that ensures that the
resulting code has a consistent format. Ie, if the new format
really were more desirable then all of the tabular forms in the
code should be made similarly illegible (oops my bias is showing)
and a space should be added before every `=' (except perhaps in
`for' statements where the current convention generally also omits
the space after `=').

Obviously one can give different weights to these criteria but since
they all agree in this case I can't see what possible excuse there
could be.

Thank you for your attention.

Ian.


--
To UNSUBSCRIBE, email to debian-dpkg-REQUEST.TakeThisOut@lists.debian.org
with a subject of "unsubscribe". Trouble? Contact listmaster.TakeThisOut@lists.debian.org

Hi Ian,

I've been meaning to send a mail to discuss the coding style issue in
the code base, and a reply to your request for indentation fixes which
was long due, but this being a very sensitive subject (as you already
acknowledged in your mail from last summer) has made me postpone it.

I'm sorry that this has been triggered from your side when doing the
merge and causing you additional work. There's some patches I've not
applied yet (which I want to, ASAP), also due to the coding style stuff
because I didn't want to keep "fixing" it w/o further discussion, or
at least a public notice.


On Wed, 2007-05-30 at 16:24:35 +0100, Ian Jackson wrote:
> While I was doing a merge of recent Debian and Ubuntu changes I found
> a conflict caused by a reforrmatting of the patch I submitted in
> #390916:

> I can see no excuse for the formatting actually applied on any of the
> conventionally applied criteria:

> * When making changes one should generally adopt the style of the
> original code.

I'd reformulate this to, when sending patches to a project one does
not have commit rights, one must try to match the existing coding
style if there's any. And I completely agree with that.


But the problem comes when one wants to change the coding style of an
existing project. I think all approaches have problems. You can fix it
in a massive commit, which destroys 'blame' and touches all files and
creates a kind of diff barrier before and after that commit, making it
both a bit useless. The other approach is incrementaly fixing the
coding style, which implies having mixed and conflicting styles on the
same code base (even on the same file!), but this does not destroy
'blame' nor does it create a diff barrier, it might just take a long
long time.

For now I've been using the latter, but maybe it makes sense to unify
everything to the same, and then we can be clear on the current style
(and also document it).

> (The original code uses tabular formatting of switch
> statements where possible

I have a problem with the "where possible", it's not consistent and
it makes it difficult to infer, more so given the mix of coding styles
in the current code base...

> and does not place a space before assignment `='.)

.... and this also applies to spaces around '=', even your reindenting
patch is not consistent in that regard, and is missing spaces after
the '='.


> * One should not change formatting without a good reason. I supplied
> a patch but the formatting actually applied was different.

The reason I've done that is because I find the current coding style
inconsistent and too packed, it cannot breath, variable and type names
massively abbreviated and missing underscores, missing blank lines
delimiting blocks, missing spaces between operators, missing spaces
after commas, multiple statements on the same line, two spaces for
indentation, etc. This makes the eyes to get tired soon.

On the perl side, some of those apply, there's also the problem of
lack of functions (which I've been fixing by refactoring for some),
and usage of old perl constructs (which I've been incrementaly fixing
as well).

> This has caused me needless work during my merge.

And I'm sorry about that.

> * The formatting should be clear and easy to read.

See two paragraphs above.

> * If reformatting, one should do so in a way that ensures that the
> resulting code has a consistent format. Ie, if the new format
> really were more desirable then all of the tabular forms in the
> code should be made similarly illegible (oops my bias is showing)
> and a space should be added before every `=' (except perhaps in
> `for' statements where the current convention generally also omits
> the space after `=').

See my comment on changing the whole coding style.


Personally I value consistency, readability and minimization of diff
size. So I'd really like to unify the style to something ressembling
the Linux CodingStyle. What do others think?


regards,
guillem


--
To UNSUBSCRIBE, email to debian-dpkg-REQUEST RemoveThis @lists.debian.org
with a subject of "unsubscribe". Trouble? Contact listmaster RemoveThis @lists.debian.org