2.0 - API design change to throw exceptions instead of returning nullables #9
Conversation
There was a problem hiding this comment.
Basically, the issue is he is wanting to expose more information in the event of failures. Which is fair. But throwing Exception everywhere is not the way to do it.
Better option is to grab the log from LogUtils and print that out when versions can't be provisioned/found.
Aside from that there are some other minor changes like the ordering of distros and nullability. Which we've discussed on discord and should be on the same page about.
| try { | ||
| return Objects.requireNonNull(readJson(tmp, TypeToken.get(DownloadInfo.class)).info); | ||
| } catch (Exception e) { | ||
| errors.add(e); |
There was a problem hiding this comment.
Reminder about Objects.requireNonNull
| error("Failed to download package info for "+ pkg.id); | ||
| return null; | ||
| error("Failed to download any packages from " + url); | ||
| throw Throwing.withSuppressed(new Exception("Failed to parse package list from " + url), errors); |
There was a problem hiding this comment.
The exception and error message are not aligned here.
| return null; | ||
| try { | ||
| DownloadUtils.tryDownloadFile(true, archive, download); | ||
| } catch (Exception e) { |
There was a problem hiding this comment.
This is broken because the return will be false, not an exception thrown.
| // Prefer TCK certified JDKs (and also test Microsoft) | ||
| public enum Distro implements Comparable<Distro> { | ||
| // These are well know/recommended distros | ||
| // These are well-known/recommended distros |
There was a problem hiding this comment.
So, discussing this on discord, just reiterating it here. I don't particularly care.
But MS > OpenJDK > Temurin > * > JDK > OpenJ9 seems to be the consensus on the order.
| log("Gradle home: \"" + gradleHome.getAbsolutePath() + "\" Does not exist"); | ||
| return; | ||
| throw new JavaLocatingFailedException("Gradle home does not exist: " + gradleHome.getAbsolutePath()); | ||
| } |
There was a problem hiding this comment.
Gradle home not existing is a valid case that shouldn't throw errors. The 'error' if any should just be because it ant find it during the find(int) function not the findAll functions.
| for (File child : root.listFiles()) { | ||
| File[] listFiles; | ||
| try { | ||
| listFiles = Objects.requireNonNull(root.listFiles()); |
There was a problem hiding this comment.
Another requiresNotNull and immediately caught exception.
| * @return null is no install found, or a File pointing at the JAVA_HOME directory | ||
| */ | ||
| File find(int version); | ||
| File find(int version) throws JavaLocatingFailedException; |
There was a problem hiding this comment.
throwing an exception here to give more information then a null would is fine.
| * @return A list containing all java installs, possibly empty but not null. | ||
| */ | ||
| List<IJavaInstall> findAll(); | ||
| FindResult findAll() throws JavaLocatingFailedException; |
There was a problem hiding this comment.
Throwing an exception here is not find as it is meant to return all available entries. Not caring for things that are broken/whatever. Its just "What are my functional options"
| List<IJavaInstall> findAll(); | ||
| FindResult findAll() throws JavaLocatingFailedException; | ||
|
|
||
| interface FindResult { |
There was a problem hiding this comment.
If you must keep this, rename to Results.
Tho I suspect this could be resolved with:
if (ret.isEmpty())
print(Log.getErrorMessages())There was a problem hiding this comment.
Ya, the interface was only because I wasn't printing the errors. I'll remove it when I redo this.
| } | ||
|
|
||
| default IJavaInstall provision(int version, Disco.Distro distro) throws JavaProvisioningFailedException { | ||
| throw new JavaProvisioningFailedException("Provisioning is not implemented by this locator"); |
There was a problem hiding this comment.
If we throw an exception here, we need to add a 'canProvision' so we don't use exception as codeflow. Tho the only thing that can provision right now is Disco.
There was a problem hiding this comment.
Would it be better to just make a subinterface? IJavaProvisioner extends IJavaLocator.
2 participants
This PR modifies the API design of Java Provisioner to throw exceptions instead of returning
nullof something goes wrong trying to locate or provision Java installations.Depends on:
Please don't squash and merge through GitHub.com. I'll merge this myself with the proper tag(s) once approval has been granted.