-
Notifications
You must be signed in to change notification settings - Fork 126
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
collects imports, removes bang line, and prepends at top of script #75
Changes from 3 commits
5c2128d
4dcd616
1c5f638
67eff8e
5ec6ed0
06b5415
e2d1794
f7d8e02
caa56e1
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -418,7 +418,7 @@ fun prepareScript(scriptResource: String, enableSupportApi: Boolean): File { | |
|
||
|
||
// support //INCLUDE directive (see https://github.com/holgerbrandl/kscript/issues/34) | ||
scriptFile = resolveIncludes(scriptFile) | ||
if (scriptFile != null) scriptFile = resolveIncludes(scriptFile) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. the scriptfile can not be null, we should rather use !! or latinit here. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You have a null check later in the code, so I thought maybe it could be null? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. file can be null as per "I do not exist" test that fails if I replace with !!, so this is necessary. |
||
|
||
// just proceed if the script file is a regular file at this point | ||
errorIf(scriptFile == null || !scriptFile.canRead()) { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -9,21 +9,20 @@ import java.net.URL | |
*/ | ||
|
||
/** Resolve include declarations in a script file. Resolved script will be put into another temporary script */ | ||
fun resolveIncludes(template: File?): File? { | ||
if (template == null) return null | ||
fun resolveIncludes(template: File): File = resolveIncludesInternal(template) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. By doing so you removed the recursiveness. Before included files could contain There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think its a YAGNI thing. Do you want it back? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I personally think it also better to leave only at the top level and not allow recursion as it avoids the possibility of circular includes. When we add the feature to allow a "preamble" file for the DSL functionality then all includes would only go in there? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, you could list includes in the preamble. And you're right about recursion YAGNIness. Not sure why I was obsessed enough in the first place to implement it that way. |
||
|
||
// recursively replace //INCLUDES | ||
return resolveIncludesInternal(template)?.run { resolveIncludes(this) } ?: template | ||
} | ||
|
||
private fun resolveIncludesInternal(template: File): File? { | ||
private fun resolveIncludesInternal(template: File): File { | ||
val scriptLines = template.readText().lines() | ||
|
||
// don't do a thing if there's not INCLUDE in the script | ||
if (scriptLines.find { it.startsWith("//INCLUDE ") } == null) return null | ||
if (scriptLines.find { it.startsWith("//INCLUDE ") } == null) { | ||
return template | ||
} | ||
|
||
val sb = StringBuilder() | ||
|
||
// collect up the set of imports in this | ||
val imports : MutableSet<String> = emptySet<String>().toMutableSet() | ||
|
||
scriptLines.map { | ||
if (it.startsWith("//INCLUDE")) { | ||
val include = it.split("[ ]+".toRegex()).last() | ||
|
@@ -37,20 +36,32 @@ private fun resolveIncludesInternal(template: File): File? { | |
} | ||
|
||
try { | ||
sb.appendln(includeURL.readText()) | ||
// collect the import or emit | ||
includeURL.readText().lines().forEach { | ||
if (it.startsWith("import")) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Do you see my point?
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You're right. It needs a trailing space. I will add one. Partition is neater and cleaner too. |
||
imports.add(it) | ||
} else { | ||
sb.appendln(it) | ||
} | ||
} | ||
} catch (e: FileNotFoundException) { | ||
errorMsg("Failed to resolve //INCLUDE '${include}'") | ||
System.err.println(e.message?.lines()!!.map { it.prependIndent("[ERROR] ") }) | ||
|
||
quit(1) | ||
} | ||
} else if (it.startsWith("import")) { | ||
imports.add(it) | ||
} else { | ||
// if it's not a directive we simply skip emit the line as it is | ||
sb.appendln(it) | ||
// if its not an include directive or an import or a bang line, emit as is | ||
if (it.startsWith("#")) { } else { sb.appendln(it) } | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. imho should be |
||
} | ||
} | ||
|
||
return createTmpScript(sb.toString()) | ||
val impsb = StringBuilder() | ||
imports.map { impsb.appendln(it) } | ||
|
||
val final = impsb.appendln(sb.toString()) | ||
return createTmpScript(final.toString()) | ||
} | ||
|
||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
always worked for me using sdk install. imho should not be part of PR since it does not relate to it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
perhaps something has changed lately with sdk install? The travis ci wont work without this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've just rerun the build using the current repo head and it run through without problems. See https://travis-ci.org/holgerbrandl/kscript/builds/309752820
So maybe it was temporary problem?