Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces Kokoro configurations and associated shell scripts for macOS external continuous, presubmit, and release builds of the Dart IntelliJ plugin. It implements dynamic plugin versioning in build.gradle.kts for dev builds, incorporating date and Git commit hash, and includes a deployment script to upload the plugin to the JetBrains Marketplace. Feedback highlights several critical issues: an incorrect setup.sh path in build.sh, missing -Pdev flags for Gradle builds in both build.sh and deploy.sh to enable dev versioning, and an incorrect version determination logic in deploy.sh for dev builds. Additionally, there are concerns regarding unidiomatic Kotlin null checks and the use of System.out.println in build.gradle.kts, which violates the repository's style guide.
| pluginVersion = "$nextMajorVersion.0.0-dev.$datestamp" | ||
|
|
||
| val commitHash = System.getenv("KOKORO_GIT_COMMIT") | ||
| if (commitHash is String) { |
There was a problem hiding this comment.
[CONCERN] Using is String to check if an environment variable exists is unidiomatic. System.getenv() returns a nullable String?. A clearer way to perform a null check in Kotlin is using != null or a scope function like .let.
if (commitHash != null) {References
- Per the style guide, use of idiomatic Kotlin, such as for null safety, is preferred. The double-bang
!!operator is forbidden, and safe calls?., elvis operator?:, or standard null checks should be used instead. Whileis Stringworks,!= nullis more direct and idiomatic for this check. (link)
There was a problem hiding this comment.
is String reads a bit better to me but I'm ambivalent.
@helin24: if you feel strongly, happy to tweak.
| println("plugin version: $pluginVersion") | ||
| println("ideaVersion: $ideaVersion") |
There was a problem hiding this comment.
[CONCERN] The repository style guide (rule #27) disallows the use of System.out.println. While this is a build script, using Gradle's own logging mechanism is a better practice for consistency and to control log levels. The logger is available by default in Gradle scripts.
logger.lifecycle("plugin version: $pluginVersion")
logger.lifecycle("ideaVersion: $ideaVersion")References
- Style guide rule Remove the rest of the reference to "com.intellij.coverage.*" from the plugin #27 states: 'Reject any use of System.out.println or System.err.println for logging.' (link)
There was a problem hiding this comment.
We might consider updating the style guide to ignore gradle files but here I do think System.outs are OK (and we have others too).
Adds kokoro support. This is largely based on what we have in the Flutter Plugin but a bit simplified since, among other things, we don't need to deal with JXBrowser licensing or download the Flutter SDK.
See: #247
Note: The initial bits were proposed by Gemini but I modified its suggestions considerably and tackled the gradle bits (where it got very confused).
Contribution guidelines:
dart format.