zabetak commented on code in PR #3591:
URL: https://github.com/apache/calcite/pull/3591#discussion_r1609519339
##########
core/build.gradle.kts:
##########
@@ -272,15 +273,18 @@ val integTestAll by tasks.registering() {
}
for (db in listOf("h2", "mysql", "oracle", "postgresql")) {
- val task = tasks.register("integTest" + db.capitalize(), Test::class) {
+ val task = tasks.register("integTest" + db.replaceFirstChar { if
(it.isLowerCase()) it.titlecase(
Review Comment:
Since we use the capitalize logic twice in here maybe worth refactoring the
logic to a separate line.
##########
Jenkinsfile:
##########
@@ -44,9 +44,9 @@ node('ubuntu') {
withEnv(["Path+JDK=$JAVA_JDK_17/bin","JAVA_HOME=$JAVA_JDK_17"]) {
withCredentials([string(credentialsId: 'SONARCLOUD_TOKEN', variable:
'SONAR_TOKEN')]) {
if ( env.BRANCH_NAME.startsWith("PR-") ) {
- sh './gradlew --no-parallel --no-daemon jacocoAggregateTestReport
sonar -PenableJacoco -Dsonar.pullrequest.branch=${CHANGE_BRANCH}
-Dsonar.pullrequest.base=${CHANGE_TARGET} -Dsonar.pullrequest.key=${CHANGE_ID}
-Dsonar.login=${SONAR_TOKEN}'
+ sh './gradlew --no-parallel --no-daemon jacocoAggregateTestReport
sonar -PenableJacoco -Porg.sonarqube.version=4.4.1.3373
-Dsonar.pullrequest.branch=${CHANGE_BRANCH}
-Dsonar.pullrequest.base=${CHANGE_TARGET} -Dsonar.pullrequest.key=${CHANGE_ID}
-Dsonar.login=${SONAR_TOKEN}'
Review Comment:
I am not sure I understood why we are not updating the version directly to
the gradle.properties file. I have the impression that running sonar with jdk8
is not supported/recommended.
##########
build.gradle.kts:
##########
@@ -66,15 +67,6 @@ repositories {
mavenCentral()
}
-tasks.wrapper {
Review Comment:
Maybe worth adding this clarification in the final commit message in case
people in the future wonder why it was removed.
##########
build.gradle.kts:
##########
@@ -965,9 +982,11 @@ allprojects {
asNode()
}
name.set(
- (project.findProperty("artifact.name") as? String)
?: "Calcite ${project.name.capitalize()}"
+ (project.findProperty("artifact.name") as? String)
?: "Calcite ${project.name.replaceFirstChar {
Review Comment:
For our context I think that `String.replaceFirstChar { it.uppercaseChar()
}` should be sufficient. We don't have exotic characters in the project name.
Also maybe worth mentioning in the commit message that we replace
`capitalize` cause it is deprecated.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
