Gh-308: Remove Guava dependency #309
Conversation
Codecov Report
📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more @@ Coverage Diff @@
## develop #309 +/- ##
=============================================
- Coverage 82.20% 82.15% -0.05%
- Complexity 1618 1626 +8
=============================================
Files 194 194
Lines 4326 4338 +12
Branches 436 436
=============================================
+ Hits 3556 3564 +8
- Misses 591 594 +3
- Partials 179 180 +1
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
There was a problem hiding this comment.
Draft because IterableUtil.java needs new unit tests adding.
These changes should also be tested with Gaffer before being merged as there's a potential that some untested behaviours (e.g. Null handling, unmodifiable) have changed compared to when Guava methods were used.
| @@ -53,7 +53,7 @@ public T apply(final Iterable<T> input) { | |||
| throw new IllegalArgumentException("Input cannot be null"); | |||
| } | |||
| try { | |||
| return Iterables.get(input, selection); | |||
| return IterableUtil.toList(input).get(selection); | |||
There was a problem hiding this comment.
This approach could be less efficient, depending on how Guava implements it.
There was a problem hiding this comment.
This is guava's implementation. Ideally we will want to do something similar as converting to a list will be less efficient if the underlying object is not a List itself.
| @@ -366,7 +367,7 @@ private static void addSimpleClassNames(final Map<String, Set<Class>> map, final | |||
| final String id = StringUtils.capitalize(entry.getKey()); | |||
| final Set<Class> value = map.get(id); | |||
| if (null == value) { | |||
| map.put(id, Sets.newHashSet(entry.getValue())); | |||
There was a problem hiding this comment.
Did this create a new Set before to effectively set by value rather than reference?
There was a problem hiding this comment.
It created a new set which was a copy of the old one. There's some similar code somewhere else which didn't make a copy so I just removed the copy.
There was a problem hiding this comment.
I just worry this would change functionality as it now sets by reference not by value? Could make a new unit test?
There was a problem hiding this comment.
Could do, though if you're concerned it would be easiest to change it back to creating a copy.
|
Further work is needed and ideally Java 8 support should be dropped first, as it would be much easier to replace some methods by moving to Java 11 where they are built in. |
Related Issue