portals-jetspeed-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From David Le Strat <dlest...@yahoo.com>
Subject Re: Code style conventions
Date Sun, 29 Jan 2006 23:34:24 GMT
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 <david_jencks@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


Mime
View raw message