Re: [PATCH v2 12/13] Add guidelines about warnings and whitespaces

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Thu, 2018-02-08 at 11:09 +0100, Victor Toso wrote:
> Hi,
> 
> On Thu, Feb 08, 2018 at 05:01:05AM -0500, Frediano Ziglio wrote:
> > > > Depends on many cases. You don't want spurious changes to make harder to
> > > > look at the history for instance (that is a point for Nack).
> > > > The patch is not fixing anything or adding new feature (another for Nack).
> > > > On the other hand applied to code not changed for a long period (where is
> > > > unlikely to have to dig the history) or code with small history (where
> > > > is faster to skip in any case) changes.
> > > > Being style it depends also on personal opinions.
> > > 
> > > You can’t have it both ways. Here, you are simultaneously saying:
> > > 
> > > - We don’t want trailing whitespace
> > 
> > OT: Not only trailing, also tabs for instance.
> > 
> > > - We nack patches that fix trailing whitespace on their own
> > 
> > Not saying that, I'm saying is not black and white.
> 
> Because the code itself is inconsistent. It would be so much
> better to have a few patches that make the code consistent and
> then some git hook to check if given patch does not mess around
> the coding style instead of discussing this so many times over
> years.

Agreed!

My (partly philosofical) opinion on this is:

Code quality is more important than history (or diffs). If something is
wrong, just go ahead and fix it. If you want it fixed, it will be
recorded in the history. It's cleaner to have a separate commit for it
in the history. (that is obvious, right?)

Lukas
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/spice-devel




[Index of Archives]     [Linux ARM Kernel]     [Linux ARM]     [Linux Omap]     [Fedora ARM]     [IETF Annouce]     [Security]     [Bugtraq]     [Linux]     [Linux OMAP]     [Linux MIPS]     [ECOS]     [Asterisk Internet PBX]     [Linux API]     [Monitors]