Re: svn commit: r990233 - /commons/proper/configuration/trunk/conf/testdb.script

Gary,

can you please stop with this code reformatting? How should anyone can
have 
a look at the changes if it is hidden between 95% unrelated stuff ???

- Jörg



--------------- Forwarded Message

Betreff: svn commit: r949140 [2/3] - in /commons/proper/lang/trunk/src: 
main/java/org/apache/commons/lang3/SystemUtils.java 
test/java/org/apache/commons/lang3/SystemUtilsTest.java
Absender: ggreg### @apache.org
Datum: Fri, 28 May 2010 10:12:04 +0000
Newsgruppe: gmane.comp.jakarta.commons.scm


Modified: 
commons/proper/lang/trunk/src/main/java/org/apache/commons/lang3/SystemUtils.java
URL: 
http://svn.apache.org/viewvc/commons/...140&view=diff

On 2 August 2010 11:08,  <joc### @apache.org> wrote:
 Author: jochen
 Date: Mon Aug  2 10:08:42 2010
 New Revision: 981442

 URL: http://svn.apache.org/viewvc?rev=981442&view=rev
 Log:
 EOL fixes

 Modified:
    commons/proper/fileupload/trunk/.project

Generally not a good idea to include Eclipse project files in SVN.

They aren't usually portable and can cause problems for users with
different Eclipse settings.

On 22 July 2010 12:35,  <jca### @apache.org> wrote:
 Author: jcarman
 Date: Thu Jul 22 11:35:07 2010
 New Revision: 966589

 URL: http://svn.apache.org/viewvc?rev=966589&view=rev
 Log:
 Misc. event utils.

 Added:
  
 commons/proper/lang/trunk/src/main/java/org/apache/commons/lang3/event/EventListenerSupport.java
  
 commons/proper/lang/trunk/src/main/java/org/apache/commons/lang3/event/EventUtils.java
  
 commons/proper/lang/trunk/src/test/java/org/apache/commons/lang3/event/EventListenerSupportTest.java
  
 commons/proper/lang/trunk/src/test/java/org/apache/commons/lang3/event/EventUtilsTest.java

You need to set svn:eol-style native for alll such files.

Looks like your SVN config is wrong:

http://www.apache.org/dev/version-con...#https-svn-config

On 10 August 2010 12:31,  <jcar### @apache.org> wrote:
 Author: jcarman
 Date: Tue Aug 10 11:31:38 2010
 New Revision: 983933

 URL: http://svn.apache.org/viewvc?rev=983933&view=rev
 Log:
 Reverting "generification" of toMap() method in ArrayUtils.

However, the changes to ArrayUtilsTest seem to have reverted other
fixes as well - was that intentional?

 Modified:
  
 commons/proper/lang/trunk/src/main/java/org/apache/commons/lang3/ArrayUtils.java
  
 commons/proper/lang/trunk/src/test/java/org/apache/commons/lang3/ArrayUtilsTest.java

 Modified:
commons/proper/lang/trunk/src/main/java/org/apache/commons/lang3/ArrayUtils.java
 URL:
http://svn.apache.org/viewvc/commons/...933&view=diff
 

I am not sure whether this is actually a problem, but I vaguely remember 
that you can have trouble with time zones when converting between dates 
and calendars. I found the following reference [1] which might be helpful.

Oliver

[1] http://www.odi.ch/prog/design/datetime.php

Am 13.07.2010 07:17, schrieb baya### @apache.org:
 Author: bayard
 Date: Tue Jul 13 05:17:48 2010
 New Revision: 963601

 URL: http://svn.apache.org/viewvc?rev=963601&view=rev
 Log:
 Adding toCalendar method per LANG-632

 Modified:
     
commons/proper/lang/trunk/src/main/java/org/apache/commons/lang3/time/DateUtils.java
     
commons/proper/lang/trunk/src/test/java/org/apache/commons/lang3/time/DateUtilsTest.java

 Modified:
commons/proper/lang/trunk/src/main/java/org/apache/commons/lang3/time/DateUtils.java
 URL:
http://svn.apache.org/viewvc/commons/...601&view=diff
 

On Sun, Aug 8, 2010 at 1:25 PM, sebb <seb### @gmail.com> wrote:

 As it stands, the code allows calls of the form:

 Map<Integer,Integer> map = ArrayUtils.toMap(new String[][]
{{"foo",
 "bar"}, {"hello", "world"}});

 without complaining.

 Failing to check the types of the array entries on creation means
that
 the error may lie undetected until much later, making it potentially
 much harder to debug.


I am not against checking against the type arguments, but this change
assumes the user won't do something stupid.  It'd be a
ClassCastException later, yes.  Having the type arguments only allows
the user not to have to cast the return type.  They can use type
inference.

On Sun, Aug 8, 2010 at 1:39 PM, sebb <sebb### @gmail.com> wrote:
> +        assertFalse(StringUtils.startsWithAny("abcxyz",
null, "xyz", "abcd"));

 Most of the above changes seem wrong to me, as they change what is
being tested.

 By all means add tests for varargs, but surely the array tests need
to
 be kept as well?


How do they change what is being tested exactly?

On 8 August 2010 21:30, James Carman <ja### @carmanconsulting.com>
wrote:
 On Sun, Aug 8, 2010 at 1:25 PM, sebb <sebb### @gmail.com> wrote:
>
> Why is it safe to suppress warnings?
>
> If it is really necessary, please document why it is safe, e.g.
as is
> done in ArrayUtils.addAll()
>

 It's safe because the code is making the assumption that the user
 isn't trying to do something stupid.

In which case, this assumption needs to be documented as a // comment
on the annotation.

Also the type requirements aren't even mentioned in the Javadoc.

On Sun, Aug 8, 2010 at 1:39 PM, sebb <seb### @gmail.com> wrote:
 StringUtils.length() is now deprecated, and is replaced by
 CharSequenceUtils.length(), but surely the deprecated methods should
 still be tested?
 The deprecation warnings can be suppressed.

 When the deprecated method is deleted, the tests can be dropped
entirely.

 If the corresponding CharSequenceUtils.length() tests don't exist,
 then they should be added.


Yes, you're right.  I didn't realize the name of the methods and what
they're testing.  I was in clean-up mode trying to remove all the
little yellow lines from the right side of my code editor.  I'll try
to revert at least these.  I don't agree that I'm changing the tests
in the varargs cases, though.

On 12 August 2010 07:47,  <joe### @apache.org> wrote:
 Author: joehni
 Date: Thu Aug 12 06:47:43 2010
 New Revision: 984655

 URL: http://svn.apache.org/viewvc?rev=984655&view=rev
 Log:
 Fix wrong cast.

What was wrong with the cast?


 Modified:
  
 commons/proper/lang/trunk/src/main/java/org/apache/commons/lang3/reflect/ConstructorUtils.java

 Modified:
commons/proper/lang/trunk/src/main/java/org/apache/commons/lang3/reflect/ConstructorUtils.java
 URL:
http://svn.apache.org/viewvc/commons/...655&view=diff
 

On 6 August 2010 23:44,  <jcar### @apache.org> wrote:
 Author: jcarman
 Date: Fri Aug  6 22:44:38 2010
 New Revision: 983137

 URL: http://svn.apache.org/viewvc?rev=983137&view=rev
 Log:
 Generifying toMap() method (adding in possibility for type inference
on return type).

 Modified:
  
 commons/proper/lang/trunk/src/main/java/org/apache/commons/lang3/ArrayUtils.java

 Modified:
commons/proper/lang/trunk/src/main/java/org/apache/commons/lang3/ArrayUtils.java
 URL:
http://svn.apache.org/viewvc/commons/...137&view=diff
 

On 7 August 2010 13:04,  <jcar### @apache.org> wrote:
 Author: jcarman
 Date: Sat Aug  7 12:04:46 2010
 New Revision: 983219

 URL: http://svn.apache.org/viewvc?rev=983219&view=rev
 Log:
 Some code clean-up.  Removing unnecessary boxing, array creation for
varargs, etc.

 Modified:
  
 commons/proper/lang/trunk/src/test/java/org/apache/commons/lang3/StringUtilsTest.java

 Modified:
commons/proper/lang/trunk/src/test/java/org/apache/commons/lang3/StringUtilsTest.java
 URL:
http://svn.apache.org/viewvc/commons/...219&view=diff
 

On Sun, Aug 8, 2010 at 1:25 PM, sebb <sebb### @gmail.com> wrote:

 Why is it safe to suppress warnings?

 If it is really necessary, please document why it is safe, e.g. as is
 done in ArrayUtils.addAll()


It's safe because the code is making the assumption that the user
isn't trying to do something stupid.

To make sure I'm actually passing in the exact same values (by letting
the compiler create the arrays using the varargs feature) I added the
following output to the startsWithAny() method:

System.out.println("startsWithAny(): Received " + string + " input
with " + (searchStrings == null ? null :
Arrays.asList(searchStrings).toString()) + " input array");

When running the old version of the test code to the new version,
there was no difference in the output.

I also ran a similar test by changing getCommonPrefix() method:

System.out.println("getCommonPrefix(): Received " + (strs == null ?
null : Arrays.asList(strs).toString()) + " input array");

Again, no changes in the output.  So, we're at least exercising the
"guts" of the methods the same.  Now I can understand that you might
be saying that we're not exercising how the code is called the same
way in both cases, but the varargs support vs. the manual array
creation is identical.  If you think we should add back in the manual
String[] stuff, I guess we could, but I really don't see much value in
it other than testing that varargs == manual String[].


On Sun, Aug 8, 2010 at 4:26 PM, James Carman
<jam### @carmanconsulting.com> wrote:
 On Sun, Aug 8, 2010 at 1:39 PM, sebb <sebb### @gmail.com> wrote:
>> +        assertFalse(StringUtils.startsWithAny("abcxyz",
null, "xyz", "abcd"));
>
> Most of the above changes seem wrong to me, as they change what
is being tested.
>
> By all means add tests for varargs, but surely the array tests
need to
> be kept as well?
>

 How do they change what is being tested exactly?

On 25 August 2010 17:15,  <mben### @apache.org> wrote:
 Author: mbenson
 Date: Wed Aug 25 16:15:44 2010
 New Revision: 989199

 URL: http://svn.apache.org/viewvc?rev=989199&view=rev
 Log:
 make SerializationUtils.clone() type-safe

 Modified:
  
 commons/proper/lang/trunk/src/main/java/org/apache/commons/lang3/SerializationUtils.java
  
 commons/proper/lang/trunk/src/test/java/org/apache/commons/lang3/SerializationUtilsTest.java

 Modified:
commons/proper/lang/trunk/src/main/java/org/apache/commons/lang3/SerializationUtils.java
 URL:
http://svn.apache.org/viewvc/commons/...199&view=diff
 

On Mon, Aug 9, 2010 at 7:32 AM, sebb <sebb### @gmail.com> wrote:
 Why not split the code into two methods:

 public static <K,V> Map<K, V>
toMap(Map.Entry<K,V>[] array)
 and
 public static <K,V> Map<K, V> toMap(Class<K>
keyType, Class<V>
 valueType, Object[][] array)

 Both methods can be made type-safe.


Well, we did just bump the version number, so we are free to do
something like that I guess.  Anyone else have any thoughts on the
subject (are you even paying attention anymore)?

On 9 August 2010 04:45, James Carman <ja### @carmanconsulting.com>
wrote:
 On Sun, Aug 8, 2010 at 10:04 PM, sebb <sebb### @gmail.com> wrote:
>
> Yes.
>
> But the current change makes it harder to find errors.
>
> Previously, the compiler would warn about unsafe casts; with the
> current implementation the warnings are suppressed.
> IMO this is a retrograde step.
>
> I expect generic library routines to create a Map that agrees
with the
> generic types, not allow the types to be violated.
>
> Generics are partly about allowing the compiler to check that
class
> casts cannot happen.
> This relies on generic methods behaving themselves, which the
current
> implementation does not.
>

 With type erasure, I believe it's impossible (correct me if I'm
 wrong)to determine the exact type of "K" and "V" in the code so that
 we could check the type safety of the keys/values passed in.  So, we
 would either have to change the method signature to:

 public Map<K,V> toMap(Class<K> keyType, Class<V>
valueType, Object[] array);

 or, we could add a warning to the documentation for the method:

 "Warning: This code does not (and cannot) check the type safety of
the
 keys/values against the method's type variables (<K,V>).
 Thus, if you
 are expecting a Map<String,String> to be returned, but you pass
in a
 MapEntry<String,Integer>[] (or the analogous Object[][]), the
method
 will not fail, but you will get a ClassCastException at runtime."

 Other than that, the only other option that I can see is to back out
 the change completely, but then we lose the "syntactic sugar" that it
 adds.  I personally like the sugar.

The problem is that this sugar rots the teeth of the generic
type-checking system.

 So, I'd say let's go with the warning in the docs.

Why not split the code into two methods:

public static <K,V> Map<K, V> toMap(Map.Entry<K,V>[]
array)
and
public static <K,V> Map<K, V> toMap(Class<K> keyType,
Class<V>
valueType, Object[][] array)

Both methods can be made type-safe.

Failing that, the current method could at least check that all pairs
are of the same types, and AFAICT one can still add the first method
as an overloaded version which is type-safe.

I think the Object[][] version should also check that the array
entries are exactly length 2 - it's nonsense to pass in a longer
array.

 

On Sun, Aug 8, 2010 at 10:04 PM, sebb <sebb### @gmail.com> wrote:

 Yes.

 But the current change makes it harder to find errors.

 Previously, the compiler would warn about unsafe casts; with the
 current implementation the warnings are suppressed.
 IMO this is a retrograde step.

 I expect generic library routines to create a Map that agrees with
the
 generic types, not allow the types to be violated.

 Generics are partly about allowing the compiler to check that class
 casts cannot happen.
 This relies on generic methods behaving themselves, which the current
 implementation does not.


With type erasure, I believe it's impossible (correct me if I'm
wrong)to determine the exact type of "K" and "V" in the code so that
we could check the type safety of the keys/values passed in.  So, we
would either have to change the method signature to:

public Map<K,V> toMap(Class<K> keyType, Class<V>
valueType, Object[] array);

or, we could add a warning to the documentation for the method:

"Warning: This code does not (and cannot) check the type safety of the
keys/values against the method's type variables (<K,V>).  Thus, if
you
are expecting a Map<String,String> to be returned, but you pass in a
MapEntry<String,Integer>[] (or the analogous Object[][]), the method
will not fail, but you will get a ClassCastException at runtime."

Other than that, the only other option that I can see is to back out
the change completely, but then we lose the "syntactic sugar" that it
adds.  I personally like the sugar.  So, I'd say let's go with the
warning in the docs.

Argh, sorry for the comment reformatting.  Let me know if somebody feels
they need me to revert and redo.  -Matt

On Aug 9, 2010, at 11:20 AM, mben### @apache.org wrote:

 Author: mbenson
 Date: Mon Aug  9 16:20:54 2010
 New Revision: 983709
 
 URL: http://svn.apache.org/viewvc?rev=983709&view=rev
 Log:
 move/comment SuppressWarnings(unchecked)/refactor surrounding code
 
 Modified:
   
commons/proper/lang/trunk/src/main/java/org/apache/commons/lang3/reflect/ConstructorUtils.java
 
 Modified:
commons/proper/lang/trunk/src/main/java/org/apache/commons/lang3/reflect/ConstructorUtils.java
 URL:
http://svn.apache.org/viewvc/commons/...709&view=diff
 

On Mon, Aug 9, 2010 at 7:32 AM, sebb <sebb### @gmail.com> wrote:
 Why not split the code into two methods:

 public static <K,V> Map<K, V>
toMap(Map.Entry<K,V>[] array)
 and
 public static <K,V> Map<K, V> toMap(Class<K>
keyType, Class<V>
 valueType, Object[][] array)


So, what would that make the "user" code look like?  The whole reason
for this, unless I'm mistaken, was to make it easy to do stuff like:

public static final Map<String,Color> COLOR_MAP =
ArrayUtils.toMap(new
Object[][] {
  { "red", Color.red },
  { "blue", Color.blue }
} );

To me, we're really trying to make it easier to use this API, right?
So, now the code would look like:

public static final Map<String,Color> COLOR_MAP =
ArrayUtils.toMap(String.class, Color.class, new Object[][] {
  { "red", Color.red },
  { "blue", Color.blue }
} );

This is one of the big gripes about how generics are handled in Java,
you have to repeat yourself so darn much.  What if we told folks to
use a new MapBuilder instead?

public static final Map<String,Color> COLOR_MAP =
MapUtils.builder().put("red", Color.red).put("blue",
Color.blue).toMap();

It seems like this "builder" stuff is getting popular these days.
What do you guys think?