You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@cordova.apache.org by GitBox <gi...@apache.org> on 2020/01/16 21:18:10 UTC
[GitHub] [cordova-android] raphinesse opened a new pull request #900:
refactor: simplify doFindLatestInstalledBuildTools
raphinesse opened a new pull request #900: refactor: simplify doFindLatestInstalledBuildTools
URL: https://github.com/apache/cordova-android/pull/900
### Motivation and Context
<!-- Why is this change required? What problem does it solve? -->
<!-- If it fixes an open issue, please link to the issue here. -->
Just minor code cleanup
### Description
<!-- Describe your changes in detail -->
- simplify doFindLatestInstalledBuildTools
- remove `getAvailableBuildTools`
- update com.g00fy2:versioncompare
### Testing
<!-- Please describe in detail how you tested your changes. -->
`npm t`
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@cordova.apache.org
For additional commands, e-mail: commits-help@cordova.apache.org
[GitHub] [cordova-android] erisu commented on a change in pull request #900:
refactor: simplify doFindLatestInstalledBuildTools
Posted by GitBox <gi...@apache.org>.
erisu commented on a change in pull request #900: refactor: simplify doFindLatestInstalledBuildTools
URL: https://github.com/apache/cordova-android/pull/900#discussion_r367699413
##########
File path: framework/cordova.gradle
##########
@@ -40,40 +40,40 @@ String doGetProjectTarget() {
return doEnsureValueExists('project.properties', props, 'target')
}
-Version[] getAvailableBuildTools() {
- def buildToolsDir = new File(getAndroidSdkDir(), "build-tools")
- buildToolsDir.list()
- .collect { new Version(it) } // Invalid inputs will be handled as 0.0.0
- .findAll { it.isHigherThan('0.0.0') }
- .sort { a, b -> a.isHigherThan(b) ? -1 : 1 }
-}
-
Boolean isVersionValid(version) {
return !(new Version(version)).isEqual('0.0.0')
}
String doFindLatestInstalledBuildTools(String minBuildToolsVersionString) {
- def availableBuildToolsVersions
+ def buildToolsDirContents
try {
- availableBuildToolsVersions = getAvailableBuildTools()
+ def buildToolsDir = new File(getAndroidSdkDir(), "build-tools")
+ buildToolsDirContents = buildToolsDir.list()
} catch (e) {
println "An exception occurred while trying to find the Android build tools."
throw e
}
- if (availableBuildToolsVersions.length > 0) {
- def highestBuildToolsVersion = availableBuildToolsVersions[0]
- if (highestBuildToolsVersion.isLowerThan(minBuildToolsVersionString)) {
- throw new RuntimeException(
- "No usable Android build tools found. Highest installed version is " +
- highestBuildToolsVersion.getOriginalString() + "; minimum version required is " +
- minBuildToolsVersionString + ".")
- }
- highestBuildToolsVersion.getOriginalString()
- } else {
+
+ def highestBuildToolsVersion = buildToolsDirContents
+ .collect { new Version(it) }
+ // Invalid inputs will be handled as 0.0.0
+ .findAll { it.isHigherThan('0.0.0') }
+ .max()
+
+ if (highestBuildToolsVersion == null) {
throw new RuntimeException(
"No installed build tools found. Install the Android build tools version " +
minBuildToolsVersionString + " or higher.")
Review comment:
Even though this like is not touched, same could apply here.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@cordova.apache.org
For additional commands, e-mail: commits-help@cordova.apache.org
[GitHub] [cordova-android] erisu commented on a change in pull request #900:
refactor: simplify doFindLatestInstalledBuildTools
Posted by GitBox <gi...@apache.org>.
erisu commented on a change in pull request #900: refactor: simplify doFindLatestInstalledBuildTools
URL: https://github.com/apache/cordova-android/pull/900#discussion_r367699413
##########
File path: framework/cordova.gradle
##########
@@ -40,40 +40,40 @@ String doGetProjectTarget() {
return doEnsureValueExists('project.properties', props, 'target')
}
-Version[] getAvailableBuildTools() {
- def buildToolsDir = new File(getAndroidSdkDir(), "build-tools")
- buildToolsDir.list()
- .collect { new Version(it) } // Invalid inputs will be handled as 0.0.0
- .findAll { it.isHigherThan('0.0.0') }
- .sort { a, b -> a.isHigherThan(b) ? -1 : 1 }
-}
-
Boolean isVersionValid(version) {
return !(new Version(version)).isEqual('0.0.0')
}
String doFindLatestInstalledBuildTools(String minBuildToolsVersionString) {
- def availableBuildToolsVersions
+ def buildToolsDirContents
try {
- availableBuildToolsVersions = getAvailableBuildTools()
+ def buildToolsDir = new File(getAndroidSdkDir(), "build-tools")
+ buildToolsDirContents = buildToolsDir.list()
} catch (e) {
println "An exception occurred while trying to find the Android build tools."
throw e
}
- if (availableBuildToolsVersions.length > 0) {
- def highestBuildToolsVersion = availableBuildToolsVersions[0]
- if (highestBuildToolsVersion.isLowerThan(minBuildToolsVersionString)) {
- throw new RuntimeException(
- "No usable Android build tools found. Highest installed version is " +
- highestBuildToolsVersion.getOriginalString() + "; minimum version required is " +
- minBuildToolsVersionString + ".")
- }
- highestBuildToolsVersion.getOriginalString()
- } else {
+
+ def highestBuildToolsVersion = buildToolsDirContents
+ .collect { new Version(it) }
+ // Invalid inputs will be handled as 0.0.0
+ .findAll { it.isHigherThan('0.0.0') }
+ .max()
+
+ if (highestBuildToolsVersion == null) {
throw new RuntimeException(
"No installed build tools found. Install the Android build tools version " +
minBuildToolsVersionString + " or higher.")
Review comment:
Even though this line is not touched, same could apply here.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@cordova.apache.org
For additional commands, e-mail: commits-help@cordova.apache.org
[GitHub] [cordova-android] raphinesse commented on a change in pull request
#900: refactor: simplify doFindLatestInstalledBuildTools
Posted by GitBox <gi...@apache.org>.
raphinesse commented on a change in pull request #900: refactor: simplify doFindLatestInstalledBuildTools
URL: https://github.com/apache/cordova-android/pull/900#discussion_r367866071
##########
File path: framework/cordova.gradle
##########
@@ -40,40 +40,40 @@ String doGetProjectTarget() {
return doEnsureValueExists('project.properties', props, 'target')
}
-Version[] getAvailableBuildTools() {
- def buildToolsDir = new File(getAndroidSdkDir(), "build-tools")
- buildToolsDir.list()
- .collect { new Version(it) } // Invalid inputs will be handled as 0.0.0
- .findAll { it.isHigherThan('0.0.0') }
- .sort { a, b -> a.isHigherThan(b) ? -1 : 1 }
-}
-
Boolean isVersionValid(version) {
return !(new Version(version)).isEqual('0.0.0')
}
String doFindLatestInstalledBuildTools(String minBuildToolsVersionString) {
- def availableBuildToolsVersions
+ def buildToolsDirContents
try {
- availableBuildToolsVersions = getAvailableBuildTools()
+ def buildToolsDir = new File(getAndroidSdkDir(), "build-tools")
+ buildToolsDirContents = buildToolsDir.list()
} catch (e) {
println "An exception occurred while trying to find the Android build tools."
throw e
}
- if (availableBuildToolsVersions.length > 0) {
- def highestBuildToolsVersion = availableBuildToolsVersions[0]
- if (highestBuildToolsVersion.isLowerThan(minBuildToolsVersionString)) {
- throw new RuntimeException(
- "No usable Android build tools found. Highest installed version is " +
- highestBuildToolsVersion.getOriginalString() + "; minimum version required is " +
- minBuildToolsVersionString + ".")
- }
- highestBuildToolsVersion.getOriginalString()
- } else {
+
+ def highestBuildToolsVersion = buildToolsDirContents
+ .collect { new Version(it) }
+ // Invalid inputs will be handled as 0.0.0
+ .findAll { it.isHigherThan('0.0.0') }
+ .max()
+
+ if (highestBuildToolsVersion == null) {
throw new RuntimeException(
"No installed build tools found. Install the Android build tools version " +
minBuildToolsVersionString + " or higher.")
}
+
+ if (highestBuildToolsVersion.isLowerThan(minBuildToolsVersionString)) {
+ throw new RuntimeException(
+ "No usable Android build tools found. Highest installed version is " +
+ highestBuildToolsVersion.getOriginalString() + "; minimum version required is " +
+ minBuildToolsVersionString + ".")
Review comment:
I pushed a commit that uses string interpolation for the error messages while still avoiding line lengths of > 80 characters. What do you think of it?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@cordova.apache.org
For additional commands, e-mail: commits-help@cordova.apache.org
[GitHub] [cordova-android] erisu commented on a change in pull request #900:
refactor: simplify doFindLatestInstalledBuildTools
Posted by GitBox <gi...@apache.org>.
erisu commented on a change in pull request #900: refactor: simplify doFindLatestInstalledBuildTools
URL: https://github.com/apache/cordova-android/pull/900#discussion_r367699210
##########
File path: framework/cordova.gradle
##########
@@ -40,40 +40,40 @@ String doGetProjectTarget() {
return doEnsureValueExists('project.properties', props, 'target')
}
-Version[] getAvailableBuildTools() {
- def buildToolsDir = new File(getAndroidSdkDir(), "build-tools")
- buildToolsDir.list()
- .collect { new Version(it) } // Invalid inputs will be handled as 0.0.0
- .findAll { it.isHigherThan('0.0.0') }
- .sort { a, b -> a.isHigherThan(b) ? -1 : 1 }
-}
-
Boolean isVersionValid(version) {
return !(new Version(version)).isEqual('0.0.0')
}
String doFindLatestInstalledBuildTools(String minBuildToolsVersionString) {
- def availableBuildToolsVersions
+ def buildToolsDirContents
try {
- availableBuildToolsVersions = getAvailableBuildTools()
+ def buildToolsDir = new File(getAndroidSdkDir(), "build-tools")
+ buildToolsDirContents = buildToolsDir.list()
} catch (e) {
println "An exception occurred while trying to find the Android build tools."
throw e
}
- if (availableBuildToolsVersions.length > 0) {
- def highestBuildToolsVersion = availableBuildToolsVersions[0]
- if (highestBuildToolsVersion.isLowerThan(minBuildToolsVersionString)) {
- throw new RuntimeException(
- "No usable Android build tools found. Highest installed version is " +
- highestBuildToolsVersion.getOriginalString() + "; minimum version required is " +
- minBuildToolsVersionString + ".")
- }
- highestBuildToolsVersion.getOriginalString()
- } else {
+
+ def highestBuildToolsVersion = buildToolsDirContents
+ .collect { new Version(it) }
+ // Invalid inputs will be handled as 0.0.0
+ .findAll { it.isHigherThan('0.0.0') }
+ .max()
+
+ if (highestBuildToolsVersion == null) {
throw new RuntimeException(
"No installed build tools found. Install the Android build tools version " +
minBuildToolsVersionString + " or higher.")
}
+
+ if (highestBuildToolsVersion.isLowerThan(minBuildToolsVersionString)) {
+ throw new RuntimeException(
+ "No usable Android build tools found. Highest installed version is " +
+ highestBuildToolsVersion.getOriginalString() + "; minimum version required is " +
+ minBuildToolsVersionString + ".")
Review comment:
For informational purposes, since Gradle scripts are written in Groovy, using double quotations gives you string interpolation. String literals works 👍
These three lines could be written in one as,
```
"No usable Android build tools found. Highest installed version is ${highestBuildToolsVersion.getOriginalString()}; minimum version required is ${minBuildToolsVersionString}."
```
If this way is preferred just because the string is long, I would suggest single quotes so there is no string interpolation.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@cordova.apache.org
For additional commands, e-mail: commits-help@cordova.apache.org
[GitHub] [cordova-android] codecov-io commented on issue #900: refactor:
simplify doFindLatestInstalledBuildTools
Posted by GitBox <gi...@apache.org>.
codecov-io commented on issue #900: refactor: simplify doFindLatestInstalledBuildTools
URL: https://github.com/apache/cordova-android/pull/900#issuecomment-575570919
# [Codecov](https://codecov.io/gh/apache/cordova-android/pull/900?src=pr&el=h1) Report
> Merging [#900](https://codecov.io/gh/apache/cordova-android/pull/900?src=pr&el=desc) into [master](https://codecov.io/gh/apache/cordova-android/commit/a95179343142318ee64a043f5b5a03063d7b016d?src=pr&el=desc) will **not change** coverage.
> The diff coverage is `n/a`.
[](https://codecov.io/gh/apache/cordova-android/pull/900?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #900 +/- ##
=======================================
Coverage 66.17% 66.17%
=======================================
Files 19 19
Lines 1839 1839
=======================================
Hits 1217 1217
Misses 622 622
```
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/cordova-android/pull/900?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/cordova-android/pull/900?src=pr&el=footer). Last update [a951793...8046808](https://codecov.io/gh/apache/cordova-android/pull/900?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@cordova.apache.org
For additional commands, e-mail: commits-help@cordova.apache.org
[GitHub] [cordova-android] raphinesse merged pull request #900: refactor:
simplify doFindLatestInstalledBuildTools
Posted by GitBox <gi...@apache.org>.
raphinesse merged pull request #900: refactor: simplify doFindLatestInstalledBuildTools
URL: https://github.com/apache/cordova-android/pull/900
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@cordova.apache.org
For additional commands, e-mail: commits-help@cordova.apache.org
[GitHub] [cordova-android] erisu commented on a change in pull request #900:
refactor: simplify doFindLatestInstalledBuildTools
Posted by GitBox <gi...@apache.org>.
erisu commented on a change in pull request #900: refactor: simplify doFindLatestInstalledBuildTools
URL: https://github.com/apache/cordova-android/pull/900#discussion_r367699413
##########
File path: framework/cordova.gradle
##########
@@ -40,40 +40,40 @@ String doGetProjectTarget() {
return doEnsureValueExists('project.properties', props, 'target')
}
-Version[] getAvailableBuildTools() {
- def buildToolsDir = new File(getAndroidSdkDir(), "build-tools")
- buildToolsDir.list()
- .collect { new Version(it) } // Invalid inputs will be handled as 0.0.0
- .findAll { it.isHigherThan('0.0.0') }
- .sort { a, b -> a.isHigherThan(b) ? -1 : 1 }
-}
-
Boolean isVersionValid(version) {
return !(new Version(version)).isEqual('0.0.0')
}
String doFindLatestInstalledBuildTools(String minBuildToolsVersionString) {
- def availableBuildToolsVersions
+ def buildToolsDirContents
try {
- availableBuildToolsVersions = getAvailableBuildTools()
+ def buildToolsDir = new File(getAndroidSdkDir(), "build-tools")
+ buildToolsDirContents = buildToolsDir.list()
} catch (e) {
println "An exception occurred while trying to find the Android build tools."
throw e
}
- if (availableBuildToolsVersions.length > 0) {
- def highestBuildToolsVersion = availableBuildToolsVersions[0]
- if (highestBuildToolsVersion.isLowerThan(minBuildToolsVersionString)) {
- throw new RuntimeException(
- "No usable Android build tools found. Highest installed version is " +
- highestBuildToolsVersion.getOriginalString() + "; minimum version required is " +
- minBuildToolsVersionString + ".")
- }
- highestBuildToolsVersion.getOriginalString()
- } else {
+
+ def highestBuildToolsVersion = buildToolsDirContents
+ .collect { new Version(it) }
+ // Invalid inputs will be handled as 0.0.0
+ .findAll { it.isHigherThan('0.0.0') }
+ .max()
+
+ if (highestBuildToolsVersion == null) {
throw new RuntimeException(
"No installed build tools found. Install the Android build tools version " +
minBuildToolsVersionString + " or higher.")
Review comment:
Even though this like is not touched, same could apply here as described below.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@cordova.apache.org
For additional commands, e-mail: commits-help@cordova.apache.org
[GitHub] [cordova-android] erisu commented on a change in pull request #900:
refactor: simplify doFindLatestInstalledBuildTools
Posted by GitBox <gi...@apache.org>.
erisu commented on a change in pull request #900: refactor: simplify doFindLatestInstalledBuildTools
URL: https://github.com/apache/cordova-android/pull/900#discussion_r367699413
##########
File path: framework/cordova.gradle
##########
@@ -40,40 +40,40 @@ String doGetProjectTarget() {
return doEnsureValueExists('project.properties', props, 'target')
}
-Version[] getAvailableBuildTools() {
- def buildToolsDir = new File(getAndroidSdkDir(), "build-tools")
- buildToolsDir.list()
- .collect { new Version(it) } // Invalid inputs will be handled as 0.0.0
- .findAll { it.isHigherThan('0.0.0') }
- .sort { a, b -> a.isHigherThan(b) ? -1 : 1 }
-}
-
Boolean isVersionValid(version) {
return !(new Version(version)).isEqual('0.0.0')
}
String doFindLatestInstalledBuildTools(String minBuildToolsVersionString) {
- def availableBuildToolsVersions
+ def buildToolsDirContents
try {
- availableBuildToolsVersions = getAvailableBuildTools()
+ def buildToolsDir = new File(getAndroidSdkDir(), "build-tools")
+ buildToolsDirContents = buildToolsDir.list()
} catch (e) {
println "An exception occurred while trying to find the Android build tools."
throw e
}
- if (availableBuildToolsVersions.length > 0) {
- def highestBuildToolsVersion = availableBuildToolsVersions[0]
- if (highestBuildToolsVersion.isLowerThan(minBuildToolsVersionString)) {
- throw new RuntimeException(
- "No usable Android build tools found. Highest installed version is " +
- highestBuildToolsVersion.getOriginalString() + "; minimum version required is " +
- minBuildToolsVersionString + ".")
- }
- highestBuildToolsVersion.getOriginalString()
- } else {
+
+ def highestBuildToolsVersion = buildToolsDirContents
+ .collect { new Version(it) }
+ // Invalid inputs will be handled as 0.0.0
+ .findAll { it.isHigherThan('0.0.0') }
+ .max()
+
+ if (highestBuildToolsVersion == null) {
throw new RuntimeException(
"No installed build tools found. Install the Android build tools version " +
minBuildToolsVersionString + " or higher.")
Review comment:
Even though this line is not touched, same could apply here as an example.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@cordova.apache.org
For additional commands, e-mail: commits-help@cordova.apache.org