Mailing-List: contact dev-help@commons.apache.org; run by ezmlm
Precedence: bulk
Reply-To: "Commons Developers List" <dev@commons.apache.org>
Received-SPF: pass (nike.apache.org: domain of thomas.neidhart@gmail.com
 designates 209.85.215.169 as permitted sender)
Message-ID: <527D0708.6080907@gmail.com>
Date: Fri, 08 Nov 2013 16:45:12 +0100
From: Thomas Neidhart <thomas.neidhart@gmail.com>
User-Agent: Mozilla/5.0 (X11; Linux i686;
 rv:24.0) Gecko/20100101 Thunderbird/24.1.0
MIME-Version: 1.0
To: Commons Developers List <dev@commons.apache.org>
Subject: Re: [VOTE] Release of Commons Collections 4.0 based on RC2
References: <527CB5F2.1060506@gmail.com> <527CEBD5.3070000@apache.org>
In-Reply-To: <527CEBD5.3070000@apache.org>
Content-Type: text/plain; charset=ISO-8859-1
Content-Transfer-Encoding: 7bit

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

On 11/08/2013 02:49 PM, Emmanuel Bourg wrote:
> I reviewed the site, the Javadoc and the test coverage. Here are
> my comments:

Hi Emmanuel,

thanks for your review, although I had hoped for something like this
after the 4.0-alpha release. Nevertheless, see my comments inline:

> * The release notes [1] states:
> 
> "This release is not source or binary compatible with v3.x."
> 
> I think this might be somewhat confusing, as one could believe the 
> version 4 conflicts with the version 3.x when they can actually
> coexist together. I would emphasize that this version lives in a
> new package and is independent of the version 3.x.

The release notes also state the following in the opening paragraph:

"As such, this release can not be considered to be a simple, drop-in,
replacement for commons-collections. To help with the migration to
this new version, the package has changed to
"org.apache.commons.collections4", thus it is possible to have both
commons-collections versions in the classpath. "

Thus, I think it is correct to say that collections4 is incompatible
with previous releases but can be used together.

> * The removal of ExtendedProperties is under the "Removed features
> now supported by the JDK" section.

Ok, commons-configuration is not part of the official JDK, we should
re-phrase the opening remark to "supported by the JDK or other 3rd
party libraries".

> * I didn't see in the release notes the minimum version of Java 
> supported. Is it Java 5 or Java 6?

The release notes open with:

This version uses the generics features of Java 5 and is not
compatible with earlier JDK versions.

but it is not clearly stated the Java 5 is the minimum supported
version. I will add a section like this:

== Compatibility ==

* Java 5 or later is required.

> * Why aren't the Closure and Predicate interfaces in the functor
> package with their implementations? The Equator interface and its
> implementation DefaultEquator are both in the functor package. This
> looks like an inconsistency.

Indeed, the Equator interface has been placed in the functor package,
although all other interfaces normally reside in the base package. We
should move it also to the base package imho.

> * org.apache.commons.collections4.Put: I don't quite understand why
> the return type of put(K, V) is object and not V. What is the
> object returned? Is it unspecified? I think it would be worth
> clarifying the Javadoc here.

The reason is explained in the javadoc of the put method and the class
javadoc of Put.

I am unsure about the use of the splitmap concept, but afaik the idea
is to have totally separate read and write interfaces.

You may have a map that transforms the input and thus returns
different objects. When you would specify the return type in the put
method, you may get runtime errors due to impossible casts.

> * org.apache.commons.collections4.Get: Why get(Object) and not
> get(K)? That's odd because entrySet() returns Set<Map.Entry<K,V>>.

I am not sure, but my first guess would be to be consistent with the
Map interface?

> * Put and Get: There isn't much Javadoc on the methods.

Well, they do the same as their counterparts in Map.

> * Trie: More documentation on the interface would be nice, so we
> can quickly understand how it works without following the link to
> Wikipedia.

The trie interface is deliberately kept small and concise, only 1
additional method is provided over the classical sorted map interface.

The prefixMap method also describes what the user may expect from it:
a sorted map of all mappings which are prefixed by the given key.

But I would happily accept patches to further improve the
documentation of these new interfaces (also related to splitmaps).

> * CollectionUtils.collate(Iterable, Iterable, boolean) has an
> extra "/**" in its description.

ok thanks for spotting this.

> * Why CollectionUtils.collate() instead of CollectionUtils.merge().
> The latter sounds more intuitive to me.

Imho, collate is the correct technical term to merge two collections
in sorted order, but merge would also be acceptable.

> * PatriciaTrie is a trivial subclass of AbstractPatriciaTrie
> specifying a KeyAnalyzer. Would that make sense to merge the two
> classes?

The original patch had support for different key types, but they have
been dropped so far as I could not figure them out to work correctly.

I wanted to keep the functionality but have a concrete class for the
case of Strings as key, thus the PatriciaTrie class. If we would merge
these two together we lose the possibility to later support for key types.

> * TransformedSortedSet.transformedSortedSet(SortedSet, Transformer)
> is not tested
> 
> * IteratorEnumeration, EntrySetMapIterator,
> AbstractMapIteratorDecorator and
> AbstractOrderedMapIteratorDecorator are not tested.
> 
> * Several paths of InstantiateFactory are untested.
> 
> * UnmodifiableBag.unmodifiableBag() isn't tested in the case of an 
> Unmodifiable bag passed as parameter.
> 
> * Why is Unmodifiable a marker interface instead of an annotation?
> If it makes the detection of unmodifiable collection easier I would
> suggest adding an isModifiable() method in CollectionUtils that
> would also detect the unmodifiable collections from the JDK.

Because the interface existed like this since 3.x or even before and
there was no proposal to change that to something different.

Adding a utility method makes sense, would you mind creating an issue
for this?

> * UnmodifiableSortedBagunmodifiableSortedBag() takes a SortedBag<E>
> but UnmodifiableBag.unmodifiableBag() takes a Bag<? extends E>,
> why?

see discussion on COLLECTIONS-485. Allowing an implicit upcast to the
base type may result in subtle errors/mistakes when accessing the
comparator of the SortedBag.

> * TransformedSortedBag.transformedSortedBag() is not tested.
> 
> * ComparatorUtils has a very low test coverage.
> 
> * There are many untested methods in MapUtils (getXXX,
> getXXXValue, toProperties)

@test-coverage:

all these classes have existed for a long time. I am happily accepting
patches to improve the test coverage, but I will not aim to get 100%
test coverage on a library that exists for > 10 years only by myself.

Thomas
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.11 (GNU/Linux)
Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/

iQIcBAEBAgAGBQJSfQcHAAoJEKQfE8mZlFKTpRAQAKwXmKSO0sH3s7KrS+5+zmeV
PWnGyHmh8KJ3+wNofuIBxs7yt+DA20SQhDZf+jN6bqNqNN7KTRNxEJesr9lH6Z8M
vsTSrUS+R9owHo22OCsfr4vbrsyHxWC4EEy8fBPTj5+VUaBPj4Bn2Pw4+FCCUFws
hJIfaBBXyeR8dZ2MyM30Zpe3YJpm7y5wonb6MCA7IY3IGRSoOROtWBov6RBczkEJ
uokcg6XjpRir6LB50OR1jBDFuNxGfZo8HELlPtRjwa7rC2YXGV+jRKXsAXM/beRo
RT4PpoP3NVzfNlQ05HFQ3cVjpu6mjGERqd+WtbMl3QfnXCuONDazsEZoCt/Ea9qz
t8s2xjZJqUNPQhvGCgga6//ZevsYfLH+82wiyAVnjPPnqGyWQ12NjygvCQ47Zj0/
mf4KQWyxluFxdoLm63o5u7A8FMA2trZeSMhq7wOhQtSUzFqf5MJIaeObmoYQspMI
7Njy/S2/GLI1T0hVJMSDDuZyCti5Ib2aVuXNfwOnxLUt/0u1wPYe5qJc0vKsLP5U
51Zs8Kl4cRi19om9g6QfO/KYT16lAdvVwLh3B1/nZVMqWzUm31Xf0i7syFbEIgvV
yCgCuR+eV4ugf143n0/X49kNiHaZoiDA0OcGynHLZZJBkyeRXPxOMg5n024mmkzB
aLiriZ6K5ogwpIGU797Y
=+YYr
-----END PGP SIGNATURE-----

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