Conversation
Codecov Report
@@ Coverage Diff @@
## master #1587 +/- ##
============================================
+ Coverage 71.03% 71.12% +0.09%
- Complexity 1337 1340 +3
============================================
Files 238 238
Lines 9083 9074 -9
Branches 781 780 -1
============================================
+ Hits 6452 6454 +2
+ Misses 2303 2293 -10
+ Partials 328 327 -1
Continue to review full report at Codecov.
|
| @Override | ||
| public IClasspathEntry[] getClasspathEntries() { | ||
| return classpathEntries; | ||
| return classpathEntries.toArray(new IClasspathEntry[0]); |
There was a problem hiding this comment.
I think this is why the arrays are used. A lot of the Eclipse API around this uses lists.
There was a problem hiding this comment.
Yeah, we need extra conversion here, but overall, looks like using a List is a win.
| serializer.saveContainer(javaProject, newContainer); | ||
| return Status.OK_STATUS; | ||
|
|
||
| } catch (Exception ex) { |
There was a problem hiding this comment.
This looks like something we should fix if possible. (not in this PR) Why are we catching java.lang.Exception here?
There was a problem hiding this comment.
sourceArtifactPathProvider is a Callable and this call() declares Exception to be thrown. We can either replace it with a com.google.common.base.Supplier, swallow any exception that happens inside and return a nullor create an interface similar toCallableand declareCoreException` to be thrown.
| @@ -54,37 +56,36 @@ protected IStatus run(IProgressMonitor monitor) { | |||
| try { | |||
| IClasspathContainer container = JavaCore.getClasspathContainer(containerPath, javaProject); | |||
| if (container instanceof LibraryClasspathContainer) { | |||
There was a problem hiding this comment.
Shouldn't the condition be negated? If so, and tests are still passing can you add one that fails on this before fixing it?
There was a problem hiding this comment.
Wow, nice catch!
5 participants
I was looking at the code in .libraries and thought I could improve readability a bit, mainly by using
Listin place of primitive Java arrays.@akerekes what do you think?