You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@cocoon.apache.org by Bertrand Delacretaz <bd...@apache.org> on 2004/04/17 16:18:40 UTC
[POLL] don't edit files just for style changes?
Lately there have been several hints that people are annoyed by commits
consisting only of style changes (rearranging imports, "cleaning up"
indents) etc.
Of course, there are always good intentions behind such changes, but
IMHO changes which make no difference to the actual code, are:
a) risky, because every time one edits a file there's a small risk of
making a change to the code without meaning it
b) a waste of our collective energy, as (hopefully) many of us are
regularly scanning CVS diff mails, and if it's just to find out that
two brackets have been moved it's not very useful.
I have three suggestions:
1) As a general rule, we should refrain from making such changes to our
code, unless there is a good reason (code change for example) to edit
the file in question.
2) When editing a file for a good reason, we should look for such
"style" problems and fix them at the same time, or as a separate
commit, but before testing our changes.
3) We should not change the indentation / code writing style (brackets
etc.) of a file when making a minor change to it.
The idea is to keep the flow of CVS diff messages as low as possible,
to save our neuronal bandwidth for more important things than the
placement of brackets or the order of imports.
WDYT?
-Bertrand
Re: [POLL] don't edit files just for style changes?
Posted by Ugo Cei <ug...@apache.org>.
Il giorno 17/apr/04, alle 16:18, Bertrand Delacretaz ha scritto:
> I have three suggestions:
>
> 1) As a general rule, we should refrain from making such changes to
> our code, unless there is a good reason (code change for example) to
> edit the file in question.
>
> 2) When editing a file for a good reason, we should look for such
> "style" problems and fix them at the same time, or as a separate
> commit, but before testing our changes.
>
> 3) We should not change the indentation / code writing style (brackets
> etc.) of a file when making a minor change to it.
>
> The idea is to keep the flow of CVS diff messages as low as possible,
> to save our neuronal bandwidth for more important things than the
> placement of brackets or the order of imports.
>
> WDYT?
>
+ 1
Ugo
Re: [POLL] don't edit files just for style changes?
Posted by Antonio Gallardo <ag...@agssa.net>.
Bruno Dumon dijo:
> On Sat, 2004-04-17 at 16:18, Bertrand Delacretaz wrote:
>> Lately there have been several hints that people are annoyed by commits
>> consisting only of style changes (rearranging imports, "cleaning up"
>> indents) etc.
>
> I think it boils down to whether the "style change" is an improvement or
> not. Every improvement is welcome.
>
> Fixing indents like:
>
> for (...) {
> statements
> }
>
> to
>
> for (...) {
> statements
> }
>
> is a clear improvement.
>
> Changing a simple if to a complex if:
>
> if (...)
> one statement
>
> to
>
> if (...) {
> one statement
> }
>
> isn't IMO. We don't have any rules for this, so one person might be
> changing them from the first style to the second style, while another
> person might be doing the reverse.
I tought we was follow the java sun standard. The ASF point to that. We
can also write:
if (...)
{
one statement
}
Part of the quality of code is to follow one code convention (whatever).
Because it make easier reading the code. Is not enough the size of the
Cocoon, the diferent code conventions make it harder to learn. Now, I
really feel like a stupid that waste his own time worthless. :-(
I will avoid doing that. I really got the message.
Best Regards,
Antonio Gallardo.
Re: [POLL] don't edit files just for style changes?
Posted by Bruno Dumon <br...@outerthought.org>.
On Sat, 2004-04-17 at 16:18, Bertrand Delacretaz wrote:
> Lately there have been several hints that people are annoyed by commits
> consisting only of style changes (rearranging imports, "cleaning up"
> indents) etc.
I think it boils down to whether the "style change" is an improvement or
not. Every improvement is welcome.
Fixing indents like:
for (...) {
statements
}
to
for (...) {
statements
}
is a clear improvement.
Changing a simple if to a complex if:
if (...)
one statement
to
if (...) {
one statement
}
isn't IMO. We don't have any rules for this, so one person might be
changing them from the first style to the second style, while another
person might be doing the reverse.
--
Bruno Dumon http://outerthought.org/
Outerthought - Open Source, Java & XML Competence Support Center
bruno@outerthought.org bruno@apache.org
Re: [POLL] don't edit files just for style changes?
Posted by Vadim Gritsenko <va...@reverycodes.com>.
Joerg Heinicke wrote:
> IMO it's only important to separate code changes clearly from style
> changes.
+1
Vadim
Re: [POLL] don't edit files just for style changes?
Posted by Bertrand Delacretaz <bd...@codeconsult.ch>.
Le 17 avr. 04, à 17:42, Antonio Gallardo a écrit :
> Joerg Heinicke dijo:
>> IMO it's only important to separate code changes clearly from style
>> changes. For an example see
>> http://thread.gmane.org/gmane.text.xml.cocoon.cvs/11901. A replacement
>> of hand-written code with commons lang code got completely lost in a
>> huge list of style changes. It was really only luck that I saw the
>> faulty change at one place when flying over the diff.
>
> I really don't understand things now. First was told me:
>
> "Please don't commit in small chunks".
>
> Now looks like it was ok. I will committ step by step if any....
I agree with Joerg about the "separation of concerns" in commits: when
possible, it is a good idea to commit different types of modifications
separately. It helps find out what is what.
-Bertrand
Re: [POLL] don't edit files just for style changes?
Posted by Joerg Heinicke <jo...@gmx.de>.
Antonio Gallardo <agallardo <at> agssa.net> writes:
> > For an example see
> > http://thread.gmane.org/gmane.text.xml.cocoon.cvs/11901. A replacement
> > of hand-written code with commons lang code got completely lost in a
> > huge list of style changes.
>
> I really don't understand things now. First was told me:
>
> "Please don't commit in small chunks".
Who said that?
> Now looks like it was ok. I will committ step by step if any.
It's just separation of concerns: one commit per concern :)
> OK. I got the message.
Please don't feel personally attacked. It was just a sample. Unfortunately I can
not point to an annonymous commit. Somebody is always culprit ;-)
Joerg
Re: [POLL] don't edit files just for style changes?
Posted by Antonio Gallardo <ag...@agssa.net>.
Joerg Heinicke dijo:
> IMO it's only important to separate code changes clearly from style
> changes. For an example see
> http://thread.gmane.org/gmane.text.xml.cocoon.cvs/11901. A replacement
> of hand-written code with commons lang code got completely lost in a
> huge list of style changes. It was really only luck that I saw the
> faulty change at one place when flying over the diff.
I really don't understand things now. First was told me:
"Please don't commit in small chunks".
Now looks like it was ok. I will committ step by step if any.
OK. I got the message.
Best Regards,
Antonio Gallardo
Re: [POLL] don't edit files just for style changes?
Posted by Joerg Heinicke <jo...@gmx.de>.
On 17.04.2004 16:18, Bertrand Delacretaz wrote:
> Lately there have been several hints that people are annoyed by commits
> consisting only of style changes (rearranging imports, "cleaning up"
> indents) etc.
>
> Of course, there are always good intentions behind such changes, but
> IMHO changes which make no difference to the actual code, are:
>
> a) risky, because every time one edits a file there's a small risk of
> making a change to the code without meaning it
>
> b) a waste of our collective energy, as (hopefully) many of us are
> regularly scanning CVS diff mails, and if it's just to find out that two
> brackets have been moved it's not very useful.
>
> I have three suggestions:
>
> 1) As a general rule, we should refrain from making such changes to our
> code, unless there is a good reason (code change for example) to edit
> the file in question.
Code quality is IMO a good reason. Therefore I have set my eclipse
compiler probably a bit more receptive than others, e.g. imports, unused
local variables and private fields, javadocs. But for using those
features the problems list must also stay at a manageable extent. This
is the only reason I touch many of the files I would not even have
looked on without the compiler warnings.
> 2) When editing a file for a good reason, we should look for such
> "style" problems and fix them at the same time, or as a separate commit,
> but before testing our changes.
If the style problem comprises not only a few lines it's important to
commit it separately IMO. Otherwise the actual code change gets lost.
> 3) We should not change the indentation / code writing style (brackets
> etc.) of a file when making a minor change to it.
+1
> The idea is to keep the flow of CVS diff messages as low as possible, to
> save our neuronal bandwidth for more important things than the placement
> of brackets or the order of imports.
>
> WDYT?
IMO it's only important to separate code changes clearly from style
changes. For an example see
http://thread.gmane.org/gmane.text.xml.cocoon.cvs/11901. A replacement
of hand-written code with commons lang code got completely lost in a
huge list of style changes. It was really only luck that I saw the
faulty change at one place when flying over the diff.
Joerg
Re: [POLL] don't edit files just for style changes?
Posted by Nicola Ken Barozzi <ni...@apache.org>.
Stefano Mazzocchi wrote:
...
> The benefit of clean style is not enough to pay the collective price of
> the reduced signal/noise ratio of the CVS diff knowledge broadcast.
+1
Personally I don't believe much in clean style... maybe because on some
projects some go round cleaning code and not doing anything concrete,
and I find it highly annoying.
--
Nicola Ken Barozzi nicolaken@apache.org
- verba volant, scripta manent -
(discussions get forgotten, just code remains)
---------------------------------------------------------------------
Re: [POLL] don't edit files just for style changes?
Posted by Stefano Mazzocchi <st...@apache.org>.
Bertrand Delacretaz wrote:
> Lately there have been several hints that people are annoyed by commits
> consisting only of style changes (rearranging imports, "cleaning up"
> indents) etc.
>
> Of course, there are always good intentions behind such changes, but
> IMHO changes which make no difference to the actual code, are:
>
> a) risky, because every time one edits a file there's a small risk of
> making a change to the code without meaning it
>
> b) a waste of our collective energy, as (hopefully) many of us are
> regularly scanning CVS diff mails, and if it's just to find out that two
> brackets have been moved it's not very useful.
>
> I have three suggestions:
>
> 1) As a general rule, we should refrain from making such changes to our
> code, unless there is a good reason (code change for example) to edit
> the file in question.
>
> 2) When editing a file for a good reason, we should look for such
> "style" problems and fix them at the same time, or as a separate commit,
> but before testing our changes.
>
> 3) We should not change the indentation / code writing style (brackets
> etc.) of a file when making a minor change to it.
>
> The idea is to keep the flow of CVS diff messages as low as possible, to
> save our neuronal bandwidth for more important things than the placement
> of brackets or the order of imports.
>
> WDYT?
Big +1, even if I find myself guilty of this.
The benefit of clean style is not enough to pay the collective price of
the reduced signal/noise ratio of the CVS diff knowledge broadcast.
If people start considering that channel to be too noisy, less people
will take a look at it, thus killing our feedback loop that stabilizes
our system.
I think that once you realize the danger of doing so, you also realize
why you shouldn't do it.
So, thanks Bertrand for keeping it up!
--
Stefano.
Re: [POLL] don't edit files just for style changes?
Posted by Bertrand Delacretaz <bd...@codeconsult.ch>.
Le 17 avr. 04, à 21:39, Tony Collen a écrit :
> ...My condition [a] on the item above was that we should try to
> eliminate people from having to make these code changes in the first
> place. We should be following the standard Java style, and most IDEs
> can help enforce this...
I don't think an IDE-based solution will work for this with our diverse
environment.
> ...Is it possible to fit a code reformatting filter between our cvs
> commit and the repository?
This was discussed before IIRC, and we agreed on not making this
compulsory or automatic.
But, as Ralph suggests in another reply, having a tool (checkstyle or
something) which people are expected to run *manually* on their code
before committing might be the best help.
This would still allow quick fixes to be done without being blocked by
style problems (like when editing older code which has "bad" style),
while letting us improve things.
In the meantime, let's keep the "style-only" CVS commits to a minimum.
I think there's agreement about this as the general rule, but, as
always, intelligent exceptions to the rule are always welcome!
-Bertrand
Re: [POLL] don't edit files just for style changes?
Posted by Tony Collen <co...@umn.edu>.
Bertrand Delacretaz wrote:
> Lately there have been several hints that people are annoyed by commits
> consisting only of style changes (rearranging imports, "cleaning up"
> indents) etc.
>
> Of course, there are always good intentions behind such changes, but
> IMHO changes which make no difference to the actual code, are:
>
> a) risky, because every time one edits a file there's a small risk of
> making a change to the code without meaning it
>
> b) a waste of our collective energy, as (hopefully) many of us are
> regularly scanning CVS diff mails, and if it's just to find out that two
> brackets have been moved it's not very useful.
>
> I have three suggestions:
>
> 1) As a general rule, we should refrain from making such changes to our
> code, unless there is a good reason (code change for example) to edit
> the file in question.
+1 on condition [a], below
>
> 2) When editing a file for a good reason, we should look for such
> "style" problems and fix them at the same time, or as a separate commit,
> but before testing our changes.
+1 as a separate commit, per Joerg's message about "losing" changes.
Then again, the commit message should clearly say what is going on.
>
> 3) We should not change the indentation / code writing style (brackets
> etc.) of a file when making a minor change to it.
+.75, what consists of a minor change? Little one-line bugfixes?
> The idea is to keep the flow of CVS diff messages as low as possible, to
> save our neuronal bandwidth for more important things than the placement
> of brackets or the order of imports.
>
> WDYT?
At first I was a little dumbfounded by the proposition, but now that I
think of it, it makes more sense, especially for people who are either
getting paid to work on Cocoon full-time, or people who are taking time
out of their busy day to contribute to the project when they could be
earning money in other ways (i.e. consulting).
My condition [a] on the item above was that we should try to eliminate
people from having to make these code changes in the first place. We
should be following the standard Java style, and most IDEs can help
enforce this.
Likewise, we could also make a run through all our code with something
as a once-over to fix most of the problems and then "highly encourage"
people to follow the Java style guidelines. I know it's probably
micromanaging people by enforcing a code convention, but I would also
like to argue that readable code which is consistently formatted is
easier to read (and maintain), and thus costs less time than wasting
scanning CVS commit messages for code which was reformatted. Remember,
a lot of work goes into maintaining a project than actually creating new
stuff.
Is it possible to fit a code reformatting filter between our cvs commit
and the repository?
>
> -Bertrand
>
Tony