"Cosmetic" changes

[Jeremy Boynes <jb...@apache.org>]

In another thread, Dan wrote:
"I also removed the modifications to TableName from the patch as they
were only cosmetic."

Not referring to these particular changes, but I'd like to open a 
discussion on other "cosmetic" changes. Digging through the codebase, I 
often see warnings or errors reported by my IDE (IdeaJ) for things like 
unused variables or methods, bad JavaDoc tags, unused imports, and so 
forth.

One annoyance here is separating these from genuine errors that occur as 
you are making changes, or as the IDE is adding markers for usages etc.

I would like to propose a gradual clean-up of these issues - a kind of 
"cosmetic" patch process which does not change any functionality but 
just tidies up code. One rule would be that these type of changes would 
not be mixed in with real patches for bug fixes or new features (which 
would fit with removing them from the Derby-13 patch).

How do people feel about this?
--
Jeremy

Re: "Cosmetic" changes

[Brian McCallister <br...@apache.org>]

On Jan 21, 2005, at 12:22 PM, Daniel John Debrunner wrote:

> B) Re-formatting. Changes in this group are a nightmare. :-(
> Because everyone has a different idea of what the correct formatting 
> is.
> It also tends to make life difficult for someone who is modifying the
> file as any format cleanup will tend to cause merge failures.
> I'm loathe to even raise this because of the endless discussions that
> can ensue.

I have had good experience with creating an ant task which reformats 
whole codebase and dictating that it be run before checkins.

This has a few benefits:

* Anyone can use their auto-reformat to format however they like when 
they work, and still bring it back to standard on checkin.

* Formatting changes are left out of diffs/updates even when it is 
otherwise unchanged but near changed code

* Ends the fight once and for all =)

Using SVN it is even nicer as it uses hashes to determine file changes 
instead of timestamps (this is painful under CVS as it uses timestamps, 
so it always looks like everything has changed).

-Brian

Re: "Cosmetic" changes - type "B"

[Jeremy Boynes <jb...@gluecode.com>]

Daniel John Debrunner wrote:
> 
> B) Re-formatting. Changes in this group are a nightmare. :-(
> Because everyone has a different idea of what the correct formatting is.
> It also tends to make life difficult for someone who is modifying the
> file as any format cleanup will tend to cause merge failures.
> I'm loathe to even raise this because of the endless discussions that
> can ensue.
> 

<warning>
Just colour should this bikeshed be? :-)
</warning>

I think the challenge here is in agreeing what the style should be - 
once that is done then reformatting the code can be done automatically 
by an IDE in a big, one-time, format-only fix done when people do not 
have a lot to merge.

Personally, I would like to know what the formatting rules are and then 
just set them up once in the tool - this means that you don't fight the 
IDE when editing. We took this option with Geronimo (as we started from 
scratch) and shorted circuited the argument by making the format "the 
default layout from your IDE" with a couple of tweaks, relying on the 
assumption the IDEs people were using (mainly Idea and Eclipse) were 
very similar.

Unfortunately, that layout is different from the layout for the majority 
of the Derby code I've seen to date so may not be acceptable or popular. 
The alternative is to publish a template for each IDE with the project's 
rules in it.

I prefer this to Brian's approach of automatic re-formatting using ant 
or a svn hook as:
1) no rule can't be broken, so having a layout that is non-standard
    sometimes make more sense
2) the "format as you type" features of IDEs (OK, IdeaJ at least) seem
    to be less agressive than tools intended for a one-shot reformat
3) there isn't the potential to forget running the tool before checkin;
    conversely, there isn't the potential for an auto-format to mess up
    something critical during the commit process

--
Jeremy

Re: "Cosmetic" changes

[Craig McClanahan <cr...@gmail.com>]

On Fri, 21 Jan 2005 09:22:33 -0800, Daniel John Debrunner
<dj...@debrunners.com> wrote:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
> 
> Kathey Marsden wrote:
> 
> I agree, there were two reasons I removed files from patches that had
> cosmetic cleanups:
> [snip]

<delurk>
A couple of notes about how other Apache projects address these issues
might be helpful:

* Cosmetic changes that bring existing code into conformance
  with the project's agreed upon coding standards (or clean up
  typos etc.) are generally welcomed, but should be *separated*
  from changes that have functional differences.

* The "conformance" statement above, of course, requires that
  the project developers have an agreed upon set of standards :-).
  Apache doesn't impose any such guidelines ... it should be up
  to the committers on a particular project.

There is also a more informal convention for functional-change
patches, that you conform to the "local" style of the surrounding
code, even if it does not currently conform to the community
standards.
</delurk>

Craig McClanahan

Re: "Cosmetic" changes

[Daniel John Debrunner <dj...@debrunners.com>]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Kathey Marsden wrote:

> I think a clean-up effort would be great. I especially think it is a
> wonderful way for folks to get their feet wet with Derby.  It is great
> to go through the process of making changes, running tests and
> submitting a patch just to get the process under one's belt.
>
> I think, however, it is ok to allow some cleanup to go along with
> patches.  Sometimes you are working in an area and see something and it
> is easy to fix it while you are there.  Of course there is a balance.
> If the cleanup makes the diff unclear, then it should wait.

I agree, there were two reasons I removed files from patches that had
cosmetic cleanups:

 1) The files only contained cosmetic changes, if they had contained
real changes as well, I would have left them in the patch. (subject to
Kathey's concern's about not confusing the patch).

 2) The changes were only re-formatting changes.

I also think that there are two types of cosmetic changes:

A) Jeremy's exmaples of code that causes warnings in IDE's, unused
variables & imports. Also variable/method renaming for clarity (ie.
better reflect the use or purpose of the item), fixing comments or
additional comments that make the code easier to understand.
 Changes in this group A) I think are a good cleanup exercise.

B) Re-formatting. Changes in this group are a nightmare. :-(
Because everyone has a different idea of what the correct formatting is.
It also tends to make life difficult for someone who is modifying the
file as any format cleanup will tend to cause merge failures.
I'm loathe to even raise this because of the endless discussions that
can ensue.

Dan.
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.2.5 (MingW32)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org

iD8DBQFB8TpZIv0S4qsbfuQRAs79AKDIV36Cf7fupYoqP0/sDYpKOjK0fwCeOFnI
b13C43y/IfAerqNCbc9GNJ4=
=7xeK
-----END PGP SIGNATURE-----

Re: "Cosmetic" changes

[Kathey Marsden <km...@Sourcery.Org>]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Jeremy Boynes wrote:
[snip]
> I would like to propose a gradual clean-up of these issues - a kind of
> "cosmetic" patch process which does not change any functionality but
> just tidies up code. One rule would be that these type of changes would
> not be mixed in with real patches for bug fixes or new features (which
> would fit with removing them from the Derby-13 patch).

>
> How do people feel about this?
> --

I think a clean-up effort would be great. I especially think it is a
wonderful way for folks to get their feet wet with Derby.  It is great
to go through the process of making changes, running tests and
submitting a patch just to get the process under one's belt.

I think, however, it is ok to allow some cleanup to go along with
patches.  Sometimes you are working in an area and see something and it
is easy to fix it while you are there.  Of course there is a balance.
If the cleanup makes the diff unclear, then it should wait.

Kathey




> Jeremy
>
>

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.2.5 (MingW32)
Comment: Using GnuPG with Thunderbird - http://enigmail.mozdev.org

iD8DBQFB8QjCG0h36bFmkocRAkcMAJ0Y3/ZcPszSwS5HwFj4hFelpN1l9ACfagzv
B2hRk+eG21igc4lCrnzggYs=
=KtzQ
-----END PGP SIGNATURE-----