-
-
Notifications
You must be signed in to change notification settings - Fork 5
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add support for JDK9 modules #169
Conversation
Codecov ReportBase: 46.11% // Head: 46.78% // Increases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## master #169 +/- ##
============================================
+ Coverage 46.11% 46.78% +0.66%
- Complexity 219 224 +5
============================================
Files 35 35
Lines 1030 1041 +11
Branches 100 103 +3
============================================
+ Hits 475 487 +12
+ Misses 534 533 -1
Partials 21 21
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
@@ -57,7 +57,7 @@ public static Compiler javac() { | |||
private final JavaCompiler compiler; | |||
private final List<Processor> processors = new ArrayList<>(); | |||
private final List<String> options = new ArrayList<>(); | |||
private @Nullable List<File> classpath; | |||
@Nullable Set<File> classpath; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not private?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was made package-private so that we can inspect the classpath in unit tests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You could use something like @VisibleForTesting
, but it's also a sign that you're testing something the wrong way. If you don't want to create a public method exposing the field, then consider using a mockito mock for the compiler object and use a mockito capture for the parameter that uses this field. That way this field can remain private.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Generally, I prefer not to use @VisibleForTesting
since it requires a dependency on Guava, which isn't really worth it. Also making it public/protected and annotating with @VisibleForTesting
will cause the field to appear in the Javadocs which IMO is a worse trade-off.
Normally I would have exposed it as a parameter and allowed it to be DIed in. However, in this case, the field needs to be lazy which isn't a good candidate for that. IMO it's slightly better than using a mock in this case.
elementary/src/main/java/com/karuslabs/elementary/Compiler.java
Outdated
Show resolved
Hide resolved
I'm going ahead and merging this in. |
This PR introduces low-level support for JDK 9 modules. Using this approach, it is possible that we do not need to introduce any additional annotations.