-
Notifications
You must be signed in to change notification settings - Fork 6
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
Java Tender Loving Care #3
base: master
Are you sure you want to change the base?
Conversation
Thank you for this PR, the Java template could use some community TLC :-) |
There are so many changes here and not all of them are going to suit every developer, so I would recommend we agree on a few of them, and get those in via separate PRs. Remember that changes need to be consistent between this project and the openfaas/templates project. What command are you running to generate your Gradle helpers? |
I've added the gradlew helpers and updated their versions in both repos. The project.version change you made here would be good to get in, if you are happy to submit that separately? |
* General cleanup and project setup * Using a multi-project build instead of independent builds * Added gradle wrapper scripts. Fixes openfaas/templates#154 * Upgraded gradle version to 6.5.1. Fixes openfaas/templates#211 * Using spotless plugin to format and check source formatting as part of build * Using spotless plugin to add & check license headers in files * Used spotless plugin to reformat the files * Cleaned up and upgraded publishing. Using maven-publish plugin * Documented changes in README * Using java 11 to compile * Scoped sign* properties to openfaas to avoid conflicts Closes #1 . Signed-off-by: Eugen Stan <[email protected]>
* Using Java 8 for compilation
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.
@alexellis I've added detailed explanations on each change step by step.
Please go through them and if you need more information I will provide them.
I do believe this PR adds a lot of good changes to the project.
I am open (encourage) to invite other java developers for review.
I am open to work with you and other people to solve reasonable change requests.
I'm willing to contribute to the project more once this PR is taken care.
I do not want my contributions to never be merged and end up irrelevant .
Looking forward for feedback,
Eugen
buildscript { | ||
group = 'com.openfaas' | ||
version = '0.1.0' | ||
} |
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.
@alexellis : We don't need to setup these inside a buildscript
.
One way of configuring common options is to use allprojects
and subprojects
.
plugins { | ||
id 'java' | ||
id 'application' | ||
id 'maven' |
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.
@alexellis : maven plugin is deprecated an being removed. maven-publish
is the right way to go.
https://docs.gradle.org/current/userguide/maven_plugin.html#header
} | ||
|
||
task javadocJar(type: Jar) { | ||
classifier = 'javadoc' |
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 way of adding a javadoc and sources jars are also deprecated .
Using an IDE with intellisense will notify you that the classifier
is deprecated.
https://docs.gradle.org/current/userguide/publishing_maven.html#publishing_maven:complete_example
mainClassName = 'App' | ||
|
||
dependencies { | ||
compile 'com.google.guava:guava:23.0' |
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.
Guava is not used in any of the code here.
Why is it a dependency of the project?
I removed it.
Also, compile is deprecated in favor of other configurations:
https://docs.gradle.org/current/userguide/java_plugin.html#tab:configurations
compile(Deprecated)
Compile time dependencies. Superseded by implementation.
|
||
dependencies { | ||
compile 'com.google.guava:guava:23.0' | ||
testCompile 'junit:junit:4.12' |
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.
junit 5 is better and since we don't have legacy tests why not use it.
https://junit.org/junit5/
package com.openfaas.entrypoint; | ||
|
||
import org.junit.jupiter.api.Test; | ||
|
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 test is being contributted by #4 .
See the diff.
} | ||
} | ||
javadoc { | ||
if(JavaVersion.current().isJava9Compatible()) { |
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 enabled html5 for JavaDoc.
The old one was html 4 I think.
It got enhanced in java 9 + .
@@ -1,6 +1,5 @@ | |||
// Copyright (c) OpenFaaS Author(s) 2020. All rights reserved. | |||
// Copyright (c) OpenFaaS Author(s) 2018. All rights reserved. |
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.
Spotless has a function to update the copyright year automatically to the current year.
It's not configured yet. It's one line.
https://github.com/diffplug/spotless/tree/main/plugin-gradle#license-header
package com.openfaas.model; | ||
|
||
import static org.junit.jupiter.api.Assertions.assertEquals; | ||
import static org.junit.jupiter.api.Assertions.assertNull; |
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.
Migrated to junit 5.
Moved to a package (spotless does not like classes without a package).
No other changes.
@@ -0,0 +1,4 @@ | |||
rootProject.name = 'openfaas-java' | |||
|
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.
Multi-project configuration.
Description
Closes #1 .
Signed-off-by: Eugen Stan [email protected]
Motivation and Context
Java templates needed some care.
After some discussions with @alexellis on Slack I've decided to help out.
Which issue(s) this PR fixes
Provided up
How Has This Been Tested?
./gradlew clean build
builds the project.The jars, source and javadoc jars are available under build/libs and have the
Types of changes
Impact to existing users
This should not impact users since the changes are only for the build part of the libraries.
No code delivered to users has been changed by this code.
Integration tests should follow.
Checklist:
git commit -s