Comments for review
diff --git a/core/script.runtime/src/kotlin/script/dependencies/dependencies.kt b/core/script.runtime/src/kotlin/script/dependencies/dependencies.kt
index 7e60c35..2a431fd 100644
--- a/core/script.runtime/src/kotlin/script/dependencies/dependencies.kt
+++ b/core/script.runtime/src/kotlin/script/dependencies/dependencies.kt
@@ -20,6 +20,15 @@
import java.io.File
+// discuss
+
+// Currently File is used to disambiguate between plain strings and paths
+// One of the ideas is to use java.nio.Path but this requires JDK 7
+// that means that script templates would require higher JDK (but since script are run by calling koltinc it seems ok to me after consideration)
+// Andrey expressed the idea that File (or Path) does not support other protocols, should we use URL/URI? (is it viable to support non-file protocols in compiler and IDE?)
+//
+// Explicitly references javaHome, what if it's not a jvm targeted script?
+// Currently it's convenient for IDE code and for the user because including jdk classes in classpath is a mess
data class ScriptDependencies(
val javaHome: File? = null,
val classpath: List<File> = emptyList(),
diff --git a/core/script.runtime/src/kotlin/script/dependencies/dependencies_deprecated.kt b/core/script.runtime/src/kotlin/script/dependencies/dependencies_deprecated.kt
index a5f063e..8cc3ce4 100644
--- a/core/script.runtime/src/kotlin/script/dependencies/dependencies_deprecated.kt
+++ b/core/script.runtime/src/kotlin/script/dependencies/dependencies_deprecated.kt
@@ -20,6 +20,7 @@
import java.io.File
+// nothing to discuss, for compatibility with previous version
@Deprecated("Deprecated API. Use ScriptDependencies class.")
interface KotlinScriptExternalDependencies : Comparable<KotlinScriptExternalDependencies> {
val javaHome: String? get() = null
diff --git a/core/script.runtime/src/kotlin/script/dependencies/experimental/AsyncDependenciesResolver.kt b/core/script.runtime/src/kotlin/script/dependencies/experimental/AsyncDependenciesResolver.kt
index 304b7cd..b1ea40e 100644
--- a/core/script.runtime/src/kotlin/script/dependencies/experimental/AsyncDependenciesResolver.kt
+++ b/core/script.runtime/src/kotlin/script/dependencies/experimental/AsyncDependenciesResolver.kt
@@ -23,6 +23,9 @@
import kotlin.script.dependencies.Environment
import kotlin.script.dependencies.ScriptContents
+// discuss
+
+// provides updates is async manner (IDE takes advantage of it being asynchronous, compiler just calls `runBlocking { resolveAsync (...) }`)
interface AsyncDependenciesResolver : DependenciesResolver {
suspend fun resolveAsync(
scriptContents: ScriptContents, environment: Environment
diff --git a/core/script.runtime/src/kotlin/script/dependencies/resolvers.kt b/core/script.runtime/src/kotlin/script/dependencies/resolvers.kt
index ff10299..e955885 100644
--- a/core/script.runtime/src/kotlin/script/dependencies/resolvers.kt
+++ b/core/script.runtime/src/kotlin/script/dependencies/resolvers.kt
@@ -21,6 +21,24 @@
import java.io.File
import kotlin.script.dependencies.DependenciesResolver.ResolveResult
+//discuss
+
+// Provides api to discover dependencies of scripts
+// Dependencies can be content-dependent
+//
+// Some concerns on naming:
+// Environment -> ScriptEnvironment (top level class with too common a name)
+// ResolveResult -> ResolutionResult
+//
+// Admittedly DependenciesResolver is too generic a name, but ScriptDependenciesResolver is already taken by deprecated interface
+// My guess is script.runtime.jar is not gonna be used as application wide dependency in users project (like stdlib)
+// but rather as a dependency for a module that contains DependenciesResolver implementation so maybe this is a non-problem
+//
+// ResolveResult contains resulting dependencies and reports (diagnostics?)
+// reports may contain errors (regardless of result being Success or Failure) which is gonna lead to compiler error and script not run/error highlighting in IDE.
+// Is this semantic reasonable? Are Success and Failure misleading names?
+// Main idea behind Failure is to be able to distinguish between scenarios where resolver could or could not return meaningful ScriptDependencies object.
+// For example, IDE can avoid repainting all external references as errors when Resolver threw an exception or is in inconsistent state.
typealias Environment = Map<String, Any?>
interface DependenciesResolver : @Suppress("DEPRECATION") ScriptDependenciesResolver {
@@ -32,6 +50,7 @@
sealed class ResolveResult {
abstract val dependencies: ScriptDependencies?
+ // reports -> diagnostics
abstract val reports: List<ScriptReport>
data class Success(
@@ -47,18 +66,23 @@
}
}
+// is File a could type to use here?
+// No way to get script name if file is not present, should add another property (val fileName: String)
interface ScriptContents {
val file: File?
val annotations: Iterable<Annotation>
val text: CharSequence?
+ // nothing to discuss, for compatibility with previous version
@Deprecated("Use DependenciesResolver interface")
data class Position(val line: Int, val col: Int)
}
+// ScriptReport -> ScriptDiagnostic
data class ScriptReport(val message: String, val severity: Severity = ScriptReport.Severity.ERROR, val position: Position? = null) {
data class Position(val startLine: Int, val startColumn: Int, val endLine: Int? = null, val endColumn: Int? = null)
enum class Severity { ERROR, WARNING, INFO, DEBUG }
}
-fun ScriptDependencies.asSuccess(): ResolveResult.Success = ResolveResult.Success(this)
\ No newline at end of file
+// should we expose this helper?
+fun ScriptDependencies.asSuccess(): ResolutionResult.Success = ResolutionResult.Success(this)
\ No newline at end of file
diff --git a/core/script.runtime/src/kotlin/script/dependencies/resolvers_deprecated.kt b/core/script.runtime/src/kotlin/script/dependencies/resolvers_deprecated.kt
index 78fc5e1..5302b09 100644
--- a/core/script.runtime/src/kotlin/script/dependencies/resolvers_deprecated.kt
+++ b/core/script.runtime/src/kotlin/script/dependencies/resolvers_deprecated.kt
@@ -21,6 +21,7 @@
import java.util.concurrent.Future
import java.util.concurrent.TimeUnit
+// nothing to discuss, for compatibility with previous version
@Deprecated("Use DependenciesResolver interface")
interface ScriptDependenciesResolver {
diff --git a/core/script.runtime/src/kotlin/script/extensions/samWithReceiver.kt b/core/script.runtime/src/kotlin/script/extensions/samWithReceiver.kt
index c22b7e0..9ffb48c 100644
--- a/core/script.runtime/src/kotlin/script/extensions/samWithReceiver.kt
+++ b/core/script.runtime/src/kotlin/script/extensions/samWithReceiver.kt
@@ -16,6 +16,11 @@
package kotlin.script.extensions
+// discuss
+
+// Is this an appropriate place to put this class?
+// Used like this: https://github.com/gradle/kotlin-dsl/blob/cb44112374e36b41732ab390531b8bc29e8de327/provider/src/main/kotlin/org/gradle/kotlin/dsl/KotlinBuildScript.kt#L35
+// Provides access to some specific compiler extension in scripts.
@Target(AnnotationTarget.CLASS)
@Retention(AnnotationRetention.RUNTIME)
annotation class SamWithReceiverAnnotations(vararg val annotations: String)
\ No newline at end of file
diff --git a/core/script.runtime/src/kotlin/script/templates/annotations.kt b/core/script.runtime/src/kotlin/script/templates/annotations.kt
index f0b83d7..b805bffc 100644
--- a/core/script.runtime/src/kotlin/script/templates/annotations.kt
+++ b/core/script.runtime/src/kotlin/script/templates/annotations.kt
@@ -22,13 +22,28 @@
import kotlin.script.dependencies.DependenciesResolver.NoDependencies
import kotlin.script.dependencies.ScriptDependenciesResolver
+// discuss
+
+// One of the requests is to allow to share compiler args via script templates: https://youtrack.jetbrains.com/issue/KT-19120
+// Making it a property of ScriptTemplateDefinition is a viable options but it doesn't allow to changes compiler arguments based on script content.
+// For example, it could be possible to allow file level annotation and use it like:
+// @file: ExtraCompilerArgs("-enableWhatever")
+
const val DEFAULT_SCRIPT_FILE_PATTERN = ".*\\.kts"
+// classes annotated with ScriptTemplateDefinition become script templates.
+// examples: https://github.com/JetBrains/kotlin/blob/5faad493b4cf7bf33bf82475e966a99a8e835720/compiler/tests/org/jetbrains/kotlin/scripts/ScriptTemplateTest.kt#L564-L601
@Target(AnnotationTarget.CLASS)
@Retention(AnnotationRetention.RUNTIME)
annotation class ScriptTemplateDefinition(val resolver: KClass<out ScriptDependenciesResolver> = NoDependencies::class,
val scriptFilePattern: String = DEFAULT_SCRIPT_FILE_PATTERN)
+// usage example: https://github.com/JetBrains/kotlin/blob/88652154c96402475d42ae0496ab0b423cc0a2b2/libraries/tools/kotlin-script-util/src/main/kotlin/org/jetbrains/kotlin/script/util/resolve.kt#L42-L42
+//
+// ScriptContents::annotations allows access to annotations that are specified on script file via '@file: []'
+// AcceptedAnnotations is intended to be put on DependenciesResolver::resolve method and limits what annotations are accessible (none if AcceptedAnnotations is not specified)
+// Do we have to limit annotations that are accessible via ScriptContents?
+// Seems a good idea since we have to load annotation instances via reflection, which maybe and overhead if resolver doesn't really care
@Target(AnnotationTarget.FUNCTION, AnnotationTarget.CLASS)
@Retention(AnnotationRetention.RUNTIME)
annotation class AcceptedAnnotations(vararg val supportedAnnotationClasses: KClass<out Annotation>)
diff --git a/core/script.runtime/src/kotlin/script/templates/standard/templates.kt b/core/script.runtime/src/kotlin/script/templates/standard/templates.kt
index b362e85..1549950 100644
--- a/core/script.runtime/src/kotlin/script/templates/standard/templates.kt
+++ b/core/script.runtime/src/kotlin/script/templates/standard/templates.kt
@@ -16,18 +16,25 @@
package kotlin.script.templates.standard
+// discuss
+//
+// These are some 'basic' script templates
+// Should we keep them here?
/**
* Basic script definition template without parameters
*/
+// didn't find any usages of this class.
public abstract class SimpleScriptTemplate()
/**
* Script definition template with standard argv-like parameter; default for regular kotlin scripts
*/
+// Used here: https://github.com/JetBrains/kotlin/blob/65dba3615c2699e35e8da65850efc97afd674ad5/compiler/frontend/src/org/jetbrains/kotlin/script/KotlinScriptDefinition.kt#L50-L50
public abstract class ScriptTemplateWithArgs(val args: Array<String>)
/**
* Script definition template with generic bindings parameter (String to Object)
*/
+// Used here: https://github.com/JetBrains/kotlin/blob/f152af6385b2a5df8e598f2a604b1e66114f860e/idea/idea-repl/src/org/jetbrains/kotlin/jsr223/KotlinJsr223StandardScriptEngineFactory4Idea.kt#L38-L38
public abstract class ScriptTemplateWithBindings(val bindings: Map<String, Any?>)
