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?>)