Code style conventions

[David Jencks <da...@yahoo.com>]

Before I start, I would like to thank David le Strat for promptly  
applying my patches for JS2-475.  I realize that must have been  
somewhat difficult.  Although it might not sound that way :-), I'd  
like to help find a way to make it easier to make patches in the  
future, not criticize what anyone has done so far.

After submitting some patches with really large amounts of  
unintentional white space differences, I finally consulted the  
portals coding guidelines at http://portals.apache.org/development/ 
code-standards.html , and now have some questions.

First of all, I don't think the advice to always use unix lf end of  
line is consistent with the apache svn policy/recommendation http:// 
www.apache.org/dev/version-control.html which requests svn:eol-style  
native.  There is a very useful file at http://www.apache.org/dev/svn- 
eol-style.txt

Second, this policy is not followed consistently.  Many of the files  
I modified have cr-lf line endings.  This can be fixed by explicitly  
setting the svn:eol-style to native but it results in large diffs.   
There are scripts that will go through and change this for all java  
files.

Thirdly, there are an enormous number of lines ending with variable  
amounts of white space.  AFAIK most modern IDEs trim this whenever  
saving: IDEA certainly does and when I used emacs I set it to do  
this.  I suggest adding "no trailing white space" to the coding  
guidelines.  This makes it extremely diffiicult to create a  
reasonable patch: I have to look in idea to see what needs fixing,  
and change it in emacs, save, and see if I broke anything in idea.   
If I make a change in idea, the resulting patch is mostly whitespace  
differences.

Fourthly, the coding guidelines indicate that javadocs are required.   
I've seen a lot of files that don't have javadocs at all, and almost  
all files I've looked at have errors in their javadocs.  Many of the  
patched files I submitted had extensive javadoc corrections which  
were not applied with the functional changes.  Does this mean that in  
fact javadoc is not expected to be correct, javadoc is deprecated,  
javadoc patches should be supplied separately from functional  
patches, or that in the case of my patches they were just too big to  
understand at once?  Clarification on the code guidelines page would  
be useful.

Lastly, there are also numerous violations of other coding standards  
in code such as:

if (foo) {
rather than
if (foo)
{

if (false=foo)
rather than
if (!foo)

and

Iterator iterator = myList.iterator();
while (iterator.hasNext())

rather than
for (Iterator iterator = myList.iterator(); iterator.hasNext(); )

Again, my corrections along these lines were not applied with the  
functional changes: does that mean that the coding standards are  
wrong, separate patches are requested for such changes, or again that  
my patches were too big?


Anyone with IDEA can fix the white space at end of line and misplaced  
{} problems with one command and a couple minutes for IDEA to  
reformat the project.  I'm happy to supply a patch but it would be  
enormous and probably painful to apply.  I'd like to know what the  
actual policy is on the other code standard violations is before I  
invest more time making changes.

many thanks,
david jencks




---------------------------------------------------------------------
To unsubscribe, e-mail: jetspeed-dev-unsubscribe@portals.apache.org
For additional commands, e-mail: jetspeed-dev-help@portals.apache.org

Re: Code style conventions

[David Sean Taylor <da...@bluesunrise.com>]

David Jencks wrote:
> 
> First of all, I don't think the advice to always use unix lf end of  
> line is consistent with the apache svn policy/recommendation http:// 
> www.apache.org/dev/version-control.html which requests svn:eol-style  
> native.  There is a very useful file at http://www.apache.org/dev/svn- 
> eol-style.

Pretty sure most of us are using Eclipse
I've checked in a sample Eclipse formatter (from Jetspeed-1 into 
Jetspeed-2) under editors/jetspeedEclipseFormat.xml

It can be installed into Eclipse and invoked via ctrl-shift-F to format 
code to our 'standard format'

> 
> Second, this policy is not followed consistently.  Many of the files  I 
> modified have cr-lf line endings.  This can be fixed by explicitly  
> setting the svn:eol-style to native but it results in large diffs.   
> There are scripts that will go through and change this for all java  files.
> 
One problem is that there are different editor plugins for Eclipse.
For example, Im pretty sure my Velocity plugin editor is really fouling 
up formatting. Also I believe tabs can sometimes cause diff problems 
(although Eclipse should handle that)

Maybe we need to standardize on our settings again
If we could have some volunteers for each favorite editor explain how to 
trim whitespace, set tabs, or set the eol style and document it on the 
site that would be great

> Thirdly, there are an enormous number of lines ending with variable  
> amounts of white space.  AFAIK most modern IDEs trim this whenever  
> saving: IDEA certainly does and when I used emacs I set it to do  this.  
> I suggest adding "no trailing white space" to the coding  guidelines.  
> This makes it extremely diffiicult to create a  reasonable patch: I have 
> to look in idea to see what needs fixing,  and change it in emacs, save, 
> and see if I broke anything in idea.   If I make a change in idea, the 
> resulting patch is mostly whitespace  differences.
> 
> Fourthly, the coding guidelines indicate that javadocs are required.   
> I've seen a lot of files that don't have javadocs at all, and almost  
> all files I've looked at have errors in their javadocs.  Many of the  
> patched files I submitted had extensive javadoc corrections which  were 
> not applied with the functional changes.  Does this mean that in  fact 
> javadoc is not expected to be correct, javadoc is deprecated,  javadoc 
> patches should be supplied separately from functional  patches, or that 
> in the case of my patches they were just too big to  understand at 
> once?  Clarification on the code guidelines page would  be useful.
> 
Yup we're guilty of missing javadocs.
Guess we have some XP advocates here
Maybe we should have a vote on it, and then if we decide to require good 
javadocs then we all need to enforce it
Im +1 on using Javadocs with well thought out comments
Its more busy work, true, but especially for interfaces I think it 
should be required

> Lastly, there are also numerous violations of other coding standards  in 
> code such as:
> 
> if (foo) {
> rather than
> if (foo)
> {
> 
Yup, its best to go one way or the other, not both
I find my self fixing those occasionally with ctrl-shift-F

> if (false=foo)
> rather than
> if (!foo)
> 
> and
> 
> Iterator iterator = myList.iterator();
> while (iterator.hasNext())
> 
> rather than
> for (Iterator iterator = myList.iterator(); iterator.hasNext(); )
> 
> Again, my corrections along these lines were not applied with the  
> functional changes: does that mean that the coding standards are  wrong, 
> separate patches are requested for such changes, or again that  my 
> patches were too big?
> 
Your patches looked fine to me.
Hate to make excuses but I had a project going into QA last week amongst 
lots of other non-jetspeed activities. I was just in a hopeless work 
overload. Im sorry, Im really trying hard but I seem to be slipping on 
my open source duties this year :( and things don't look any better this 
week.
Really meant to get to your patches. Good thing DLS stepped in!
Thanks David

> 
> Anyone with IDEA can fix the white space at end of line and misplaced  
> {} problems with one command and a couple minutes for IDEA to  reformat 
> the project.  I'm happy to supply a patch but it would be  enormous and 
> probably painful to apply.  I'd like to know what the  actual policy is 
> on the other code standard violations is before I  invest more time 
> making changes.
> 
Again, most of use Eclipse.
Maybe an Eclipse guru can step up and
We could also bring back checkstyle. We were using checkstyle plugins 
for Maven for a while, but it seemed to have fallen off.



-- 
David Sean Taylor
Bluesunrise Software
david@bluesunrise.com
[office] +01 707 773-4646
[mobile] +01 707 529 9194

---------------------------------------------------------------------
To unsubscribe, e-mail: jetspeed-dev-unsubscribe@portals.apache.org
For additional commands, e-mail: jetspeed-dev-help@portals.apache.org

Re: Code style conventions

[David Le Strat <dl...@yahoo.com>]

David,

Thanks for the feedback.  In addition to your
comments, I think the key issue here is the time delay
between your patch submission and when we get to apply
it.  Your patches are pretty large (and of great
quality in my opinion); they are also related to an
area that has been changing over the last few weeks. 
As a result, the patches get out of sync with the
latest trunk.  You end up spending time getting them
in sync and keeping up with changes and we,
committers, end up spending time figuring out what to
apply (therefore the misses).  I think we need to work
closer and apply your patches faster.  I will apply
your latest JS2-475 tonight.

My 2 cents.

Regards,

David Le Strat

--- David Jencks <da...@yahoo.com> wrote:

> Before I start, I would like to thank David le Strat
> for promptly  
> applying my patches for JS2-475.  I realize that
> must have been  
> somewhat difficult.  Although it might not sound
> that way :-), I'd  
> like to help find a way to make it easier to make
> patches in the  
> future, not criticize what anyone has done so far.
> 
> After submitting some patches with really large
> amounts of  
> unintentional white space differences, I finally
> consulted the  
> portals coding guidelines at
> http://portals.apache.org/development/ 
> code-standards.html , and now have some questions.
> 
> First of all, I don't think the advice to always use
> unix lf end of  
> line is consistent with the apache svn
> policy/recommendation http:// 
> www.apache.org/dev/version-control.html which
> requests svn:eol-style  
> native.  There is a very useful file at
> http://www.apache.org/dev/svn- 
> eol-style.txt
> 
> Second, this policy is not followed consistently. 
> Many of the files  
> I modified have cr-lf line endings.  This can be
> fixed by explicitly  
> setting the svn:eol-style to native but it results
> in large diffs.   
> There are scripts that will go through and change
> this for all java  
> files.
> 
> Thirdly, there are an enormous number of lines
> ending with variable  
> amounts of white space.  AFAIK most modern IDEs trim
> this whenever  
> saving: IDEA certainly does and when I used emacs I
> set it to do  
> this.  I suggest adding "no trailing white space" to
> the coding  
> guidelines.  This makes it extremely diffiicult to
> create a  
> reasonable patch: I have to look in idea to see what
> needs fixing,  
> and change it in emacs, save, and see if I broke
> anything in idea.   
> If I make a change in idea, the resulting patch is
> mostly whitespace  
> differences.
> 
> Fourthly, the coding guidelines indicate that
> javadocs are required.   
> I've seen a lot of files that don't have javadocs at
> all, and almost  
> all files I've looked at have errors in their
> javadocs.  Many of the  
> patched files I submitted had extensive javadoc
> corrections which  
> were not applied with the functional changes.  Does
> this mean that in  
> fact javadoc is not expected to be correct, javadoc
> is deprecated,  
> javadoc patches should be supplied separately from
> functional  
> patches, or that in the case of my patches they were
> just too big to  
> understand at once?  Clarification on the code
> guidelines page would  
> be useful.
> 
> Lastly, there are also numerous violations of other
> coding standards  
> in code such as:
> 
> if (foo) {
> rather than
> if (foo)
> {
> 
> if (false=foo)
> rather than
> if (!foo)
> 
> and
> 
> Iterator iterator = myList.iterator();
> while (iterator.hasNext())
> 
> rather than
> for (Iterator iterator = myList.iterator();
> iterator.hasNext(); )
> 
> Again, my corrections along these lines were not
> applied with the  
> functional changes: does that mean that the coding
> standards are  
> wrong, separate patches are requested for such
> changes, or again that  
> my patches were too big?
> 
> 
> Anyone with IDEA can fix the white space at end of
> line and misplaced  
> {} problems with one command and a couple minutes
> for IDEA to  
> reformat the project.  I'm happy to supply a patch
> but it would be  
> enormous and probably painful to apply.  I'd like to
> know what the  
> actual policy is on the other code standard
> violations is before I  
> invest more time making changes.
> 
> many thanks,
> david jencks
> 
> 
> 
> 
>
---------------------------------------------------------------------
> To unsubscribe, e-mail:
> jetspeed-dev-unsubscribe@portals.apache.org
> For additional commands, e-mail:
> jetspeed-dev-help@portals.apache.org
> 
> 


________________________
David Le Strat
Blogging @ http://dlsthoughts.blogspot.com

__________________________________________________
Do You Yahoo!?
Tired of spam?  Yahoo! Mail has the best spam protection around 
http://mail.yahoo.com 

---------------------------------------------------------------------
To unsubscribe, e-mail: jetspeed-dev-unsubscribe@portals.apache.org
For additional commands, e-mail: jetspeed-dev-help@portals.apache.org