-
Notifications
You must be signed in to change notification settings - Fork 30
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
Dependency, Java and gradle version update #22
base: master
Are you sure you want to change the base?
Conversation
Updated dependenies and gradle distribution. Jetbrains annotation library
apply plugin: 'groovy' | ||
apply plugin: 'idea' | ||
apply plugin: 'signing' | ||
|
||
buildscript { | ||
repositories { mavenCentral() } | ||
dependencies { classpath 'org.eclipse.jgit:org.eclipse.jgit:3.1.0.201310021548-r' } | ||
// dependencies { classpath 'org.eclipse.jgit:org.eclipse.jgit:3.1.0.201310021548-r' } |
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.
Please remove this line, if dependency is unused inside build.gradle
@@ -23,7 +24,7 @@ ext { | |||
licenseUrl = 'http://www.apache.org/licenses/LICENSE-2.0.txt' | |||
localGHPagesRepoDir = '../grain.gh-pages' | |||
localGHPagesRepoCanonicalPath = new File(localGHPagesRepoDir).canonicalPath | |||
javaCompatibilityVersion = 1.7 | |||
javaCompatibilityVersion = 1.8 |
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 do you increase requirement for minimum Java version to 1.8?
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? Java 7 is not supported anymore. On the other hand JDK 8 has a lot of very convenient features that are useful even in groovy.
For current code it does not matter though.
P. S. Hey people, Java 9 is coming! Java 7 is 6 years old.
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.
Yes, the point is for current code we don't use features of Java 8, so its better to support both as it doesn't "cost" us anything, but might be useful for more users.
@@ -19,9 +19,8 @@ package com.sysgears.grain.highlight | |||
import com.sysgears.grain.annotations.Uncached | |||
import com.sysgears.grain.config.Config | |||
import com.sysgears.grain.taglib.GrainUtils | |||
import com.sysgears.grain.taglib.Site | |||
import org.jetbrains.annotations.Nullable |
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 to use JetBrains annotations instead of Javax, they look vendor-specific. Why do you say, that javax annotations are obsolete?
if (!currentVersion.isInterchangeable(themeVersion)) { | ||
log.warn("Current Grain version does not match the theme version: " + | ||
"found ${currentVersion}, required ${themeVersion}. Please continue with caution.") | ||
if (!currentVersion.isBackwardCompatible(themeVersion)) { |
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.
I think we don't need to validate Grain version at all, I think these check need to be just removed
An update on #13
Jetbrain annotations instead of obsolete
javax
.'application.properties' made optional (fix for #20).