You are viewing a plain text version of this content. The canonical link for it is here.
Posted to oak-dev@jackrabbit.apache.org by Chetan Mehrotra <ch...@gmail.com> on 2015/10/23 12:08:43 UTC
Re: svn commit: r1710162 - in /jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/oak:
api/PropertyState.java api/Tree.java api/Type.java api/package-info.java
plugins/memory/ModifiedNodeState.java plugins/memory/package-info.java
On Fri, Oct 23, 2015 at 3:15 PM, <fr...@apache.org> wrote:
> public final class Type<T> implements Comparable<Type<?>> {
>
> - private static final Map<String, Type<?>> TYPES = newHashMap();
> + private static final Map<String, Type<?>> TYPES = new HashMap<String, Type<?>>();
>
> private static <T> Type<T> create(int tag, boolean array, String string) {
> Type<T> type = new Type<T>(tag, array, string);
> @@ -242,10 +237,23 @@ public final class Type<T> implements Co
>
> @Override
> public int compareTo(@Nonnull Type<?> that) {
> - return ComparisonChain.start()
> - .compare(tag, that.tag)
> - .compareFalseFirst(array, that.array)
> - .result();
> + if (tag < that.tag) {
> + return -1;
> + }
> +
> + if (tag > that.tag) {
> + return 1;
> + }
> +
> + if (!array && that.array) {
> + return -1;
> + }
> +
> + if (array && !that.array) {
> + return 1;
> + }
> +
> + return 0;
> }
I am fine with removing dependency on Guava from API but not sure if
we should remove it from implementation side
Also it would be good to have a testcase to validate the above logic
if we are removing dependency from a tested Guava utility method
Chetan Mehrotra
Re: svn commit: r1710162 - in
/jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/oak:
api/PropertyState.java api/Tree.java api/Type.java api/package-info.java
plugins/memory/ModifiedNodeState.java plugins/memory/package-info.java
Posted by Angela Schreiber <an...@adobe.com>.
hi francesco
i disagree with your statement below. having tests even for trivial
changes just limits the risk of regressions now and for any later
modifications... hoping for others having covered your code with
some test, is IMO not good enough :-)
so, please add some tests or verify that it's actually covered
by some others (intelliJ actually allows you to visualise that
pretty easily when running tests with colllecting test-coverage).
kind regards
angela
On 23/10/15 12:13, "Francesco Mari" <ma...@gmail.com> wrote:
>I don't think that in this case a test is necessary, given the
>triviality of the change. Type is used by almost the whole stack, and
>the fixes are about the comparison between types. If I introduced a
>regression, it should have been caught by at least one test
>(hopefully).
>
>2015-10-23 12:08 GMT+02:00 Chetan Mehrotra <ch...@gmail.com>:
>> On Fri, Oct 23, 2015 at 3:15 PM, <fr...@apache.org> wrote:
>>> public final class Type<T> implements Comparable<Type<?>> {
>>>
>>> - private static final Map<String, Type<?>> TYPES = newHashMap();
>>> + private static final Map<String, Type<?>> TYPES = new
>>>HashMap<String, Type<?>>();
>>>
>>> private static <T> Type<T> create(int tag, boolean array, String
>>>string) {
>>> Type<T> type = new Type<T>(tag, array, string);
>>> @@ -242,10 +237,23 @@ public final class Type<T> implements Co
>>>
>>> @Override
>>> public int compareTo(@Nonnull Type<?> that) {
>>> - return ComparisonChain.start()
>>> - .compare(tag, that.tag)
>>> - .compareFalseFirst(array, that.array)
>>> - .result();
>>> + if (tag < that.tag) {
>>> + return -1;
>>> + }
>>> +
>>> + if (tag > that.tag) {
>>> + return 1;
>>> + }
>>> +
>>> + if (!array && that.array) {
>>> + return -1;
>>> + }
>>> +
>>> + if (array && !that.array) {
>>> + return 1;
>>> + }
>>> +
>>> + return 0;
>>> }
>>
>> I am fine with removing dependency on Guava from API but not sure if
>> we should remove it from implementation side
>>
>> Also it would be good to have a testcase to validate the above logic
>> if we are removing dependency from a tested Guava utility method
>>
>> Chetan Mehrotra
Re: svn commit: r1710162 - in /jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/oak:
api/PropertyState.java api/Tree.java api/Type.java api/package-info.java
plugins/memory/ModifiedNodeState.java plugins/memory/package-info.java
Posted by Francesco Mari <ma...@gmail.com>.
I don't think that in this case a test is necessary, given the
triviality of the change. Type is used by almost the whole stack, and
the fixes are about the comparison between types. If I introduced a
regression, it should have been caught by at least one test
(hopefully).
2015-10-23 12:08 GMT+02:00 Chetan Mehrotra <ch...@gmail.com>:
> On Fri, Oct 23, 2015 at 3:15 PM, <fr...@apache.org> wrote:
>> public final class Type<T> implements Comparable<Type<?>> {
>>
>> - private static final Map<String, Type<?>> TYPES = newHashMap();
>> + private static final Map<String, Type<?>> TYPES = new HashMap<String, Type<?>>();
>>
>> private static <T> Type<T> create(int tag, boolean array, String string) {
>> Type<T> type = new Type<T>(tag, array, string);
>> @@ -242,10 +237,23 @@ public final class Type<T> implements Co
>>
>> @Override
>> public int compareTo(@Nonnull Type<?> that) {
>> - return ComparisonChain.start()
>> - .compare(tag, that.tag)
>> - .compareFalseFirst(array, that.array)
>> - .result();
>> + if (tag < that.tag) {
>> + return -1;
>> + }
>> +
>> + if (tag > that.tag) {
>> + return 1;
>> + }
>> +
>> + if (!array && that.array) {
>> + return -1;
>> + }
>> +
>> + if (array && !that.array) {
>> + return 1;
>> + }
>> +
>> + return 0;
>> }
>
> I am fine with removing dependency on Guava from API but not sure if
> we should remove it from implementation side
>
> Also it would be good to have a testcase to validate the above logic
> if we are removing dependency from a tested Guava utility method
>
> Chetan Mehrotra