Re: [xwiki-devs] [xwiki-notifications] r21910 - in platform/core/trunk: xwiki-cache/xwiki-cache-tests/src/main/java/org/xwiki/cache/tests xwiki-component/xwiki-component-api/src/main/java/org/xwiki/component/manager xwiki-component/xwiki-component-default xwiki-component/xwiki-component-default/src/main/java/org/xwiki/component/embed xwiki-component/xwiki-component-default/src/main/java/org/xwiki/component/internal xwiki-component/xwiki-component-default/src/main/java/org/xwiki/component/manager xwiki-component/xwiki-component-default/src/test/java/org/xwiki/component/embed xwiki-component/xwiki-component-default/src/test/java/org/xwiki/component/manager xwiki-core/src/test/java/com/xpn/xwiki/test xwiki-plexus/src/main/java/org/xwiki/plexus/manager xwiki-rendering/xwiki-rendering-api/src/test/java/org/xwiki/rendering/internal/parser xwiki-rendering/xwiki-rendering-macros/xwiki-rendering-macro-groovy/src/test/java/org/xwiki/rendering/macro/groovy xwiki-rendering/xwiki-rendering-macros/xwiki-rendering-macro-html/src/test/java/org/xwiki/rendering xwiki-rendering/xwiki-rendering-macros/xwiki-rendering-macro-include/src/test/java/org/xwiki/rendering/internal/macro xwiki-rendering/xwiki-rendering-macros/xwiki-rendering-macro-rss/src/test/java/org/xwiki/rendering xwiki-rendering/xwiki-rendering-macros/xwiki-rendering-macro-script/src/test/java/org/xwiki/rendering/macro/script xwiki-rendering/xwiki-rendering-macros/xwiki-rendering-macro-velocity/src/test/java/org/xwiki/rendering/macro/velocity xwiki-shared-tests/src/main/java/org/xwiki/test

Tue, 14 Jul 2009 00:07:03 -0700

On Jul 14, 2009, at 8:02 AM, Marius Dumitru Florea wrote:

>
>
> Vincent Massol wrote:
>> On Jul 13, 2009, at 11:25 PM, Anca Paula Luca wrote:
>>
>>> Sergiu Dumitriu wrote:
>>>> Vincent Massol wrote:
>>>>> Hi Thomas,
>>>>>
>>>>> On Jul 13, 2009, at 3:20 PM, tmortagne (SVN) wrote:
>>>>>
>>>>>> Author: tmortagne
>>>>>> Date: 2009-07-13 15:20:03 +0200 (Mon, 13 Jul 2009)
>>>>>> New Revision: 21910
>>>>>>
>>>>> General comments:
>>>>> - Would be great if you could separate code reformatting from code
>>>>> changes. It makes it hard to read (I didn't read the commit fully
>>>>> as a
>>>>> consequence).
>>>> +10
>>>>
>>>>> - The code style has broken my styles in lots of places (see below
>>>>> for
>>>>> some comments, I haven't commented every single place it broke
>>>>> voluntary formatting).
>>>>>
>>>>> I'd like to vote for not applying code style blindly in the
>>>>> future. It
>>>>> breaks styles in lots of places and I hate it when I spend a good
>>>>> amount of my time to align code properly and it breaks it for
>>>>> producing suboptimal styling...
>>>> There are two things to balance here:
>>>> - forgetting to format the code manually
>>>> - suboptimal formatting done automatically
>>>>
>>>> I for one prefer to have more codestyle-compliant code than less,  
>>>> and
>>>> automatic application does this.
>>> I agree with Sergiu here, maybe, if automatic is suboptimal, we need
>>> to revisit
>>> our codestyle files. It's quite difficult to only format pieces of a
>>> file (one's
>>> changes), and it often results in forgetting to do it.
>>
>> codestyle files are always suboptimal since editors don't support all
>> possible code styles. For ex the eclipse one is wrong in several
>> instances (the IDEA one was better since IDEA supported more options
>> in the past).
>>
>> In any case you cannot get perfect code styles with any IDE. (This is
>> btw why I always format manually as I type).
>
> Yes, but you can manually split a long line in multiple lines by using
> additional variables to store intermediate results and using a
> StringBuffer to concatenate strings. IMO if the code formatter fails  
> to
> give the optimal result then you should adjust your code.

I don't agree with this in lots of case and especially in the case at  
hand for tests.

BTW the compiler optimizes the concatenation of strings automatically  
and performance wise it's faster to do it with concatenation than with  
String Buffer. The case when it's better to use String buffer is when  
there are variables.

> Manual formatting can leave a lot of white spaces (especially at the  
> end
> of lines) which the code formatter can easily fix.

This should be checked with checkstyle or some tool like this to  
ensure we don't have this. this is the only way to ensure we don't  
have spaces at end of lines. Formatting in IDE is only a best effort.

Thanks
-Vincent

>> All I'm asking is that people stop blindly reformatting other  
>> people's
>> code. This means that you're allowed to use automatic reformatting  
>> but
>> you must check thereafter that no weirdies were introduced and
>> manually fix introduced "errors".
>>
>> To see the kind of "errors" I'm talking about see my specific  
>> comments
>> to thomas' commit.
>>
>> Thanks
>> -Vincent
>>
>>> Happy coding,
>>> Anca
>>>
>>>> The way I do commits is:
>>>>
>>>> - change the code
>>>> - check what would the commit look like (using git diff)
>>>> - if I detect something wrong in the codestyle, I selectively  
>>>> accept
>>>> changes in the commit, using git add --interactive, which allows me
>>>> to
>>>> choose which files to commit, and even inside a file, which changes
>>>> - final check on the prepared commit using git diff --cached
>>>>
>>>> This process takes a while, but it ensures high quality commits.
_______________________________________________
devs mailing list
[email protected]
http://lists.xwiki.org/mailman/listinfo/devs

Reply via email to