-
Notifications
You must be signed in to change notification settings - Fork 78
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
SDK-311: Using Java 8 Optional Class #227
base: master
Are you sure you want to change the base?
Conversation
What do you think about these changes in relation to these two articles? |
@dkayiwa I made some changes to the code. |
Are all these changes supposed to be in this pull request? |
Ohh, I think it is a problem with rebasing. I'll recheck |
cee0f22
to
b6f254b
Compare
@dkayiwa please review these new changes. |
Feel free to continue with other pull requests as you wait for this to be reviewed. 😊 |
return mergeArtifactLists(childArtifacts, parentArtifacts); | ||
} | ||
|
||
public String getPlatformVersion(DistroHelper distroHelper, File directory) throws MojoExecutionException{ | ||
Artifact artifact = getDistroArtifact(); | ||
if (artifact != null) { | ||
DistroProperties distroProperties = distroHelper.downloadDistroProperties(directory, artifact); |
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.
How is this change related to the Java 8 Optional class?
@@ -524,13 +495,7 @@ public void runSystemNpmCommandWithArgs(List<String> arguments) throws MojoExecu | |||
} | |||
|
|||
private String getNpmSystemExecutable() { | |||
String npm; | |||
if (SystemUtils.IS_OS_WINDOWS) { |
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.
Is this also related to the Optional class?
} | ||
|
||
public PackageJson getPackageJson(String jsonFilename) throws MojoExecutionException { | ||
Reader reader = null; | ||
File json; | ||
if (mavenProject == null) { | ||
json = new File(jsonFilename); |
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.
Is this also related to the Optional class?
@wikumChamith are all the changes in this pull request related to the Optional class? |
@dkayiwa I made the changes mentioned above according to the 12th recipe of this article you shared. |
Did you see my other comments in this pull request? |
Yes. I used the ternary operator because it is better than using if-else or optional ifPresent-get. It is also recommended here: https://blogs.oracle.com/javamagazine/post/12-recipes-for-using-the-optional-class-as-its-meant-to-be-used |
Jira ticket: https://issues.openmrs.org/browse/SDK-311