collects imports, removes bang line, and prepends at top of script #75

ilanpillemer · 2017-11-30T21:51:52Z

fixes #72

holgerbrandl · 2017-11-30T23:46:45Z

src/main/kotlin/kscript/app/Kscript.kt

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


the scriptfile can not be null, we should rather use !! or latinit here.

You have a null check later in the code, so I thought maybe it could be null?

file can be null as per "I do not exist" test that fails if I replace with !!, so this is necessary.

holgerbrandl · 2017-11-30T23:37:17Z

.travis.yml

@@ -26,6 +26,7 @@ install:
  - sudo add-apt-repository -y ppa:openjdk-r/ppa
  - sudo apt-get update
  - sudo apt-get install openjdk-8-jdk
+  - sudo apt-get install maven2


always worked for me using sdk install. imho should not be part of PR since it does not relate to it.

perhaps something has changed lately with sdk install? The travis ci wont work without this?

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?

holgerbrandl · 2017-11-30T23:54:47Z

src/main/kotlin/kscript/app/ResolveIncludes.kt

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


By doing so you removed the recursiveness. Before included files could contain @Includes themselfes, but you refactored that away. But maybe this was too far-fetched anyway.

I think its a YAGNI thing. Do you want it back?

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?

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.

holgerbrandl · 2017-11-30T23:57:16Z

src/main/kotlin/kscript/app/ResolveIncludes.kt

-                sb.appendln(includeURL.readText())
+                // collect the import or emit
+                includeURL.readText().lines().forEach {
+                    if (it.startsWith("import")) {


it must be "import " at least (with a tailing space) to make it more robust. Otherwise we might mess up logic such as

fun importStuff() = "" importStuff()

Do you see my point?
I'm still a bit worried that such a predicate might create false positives, but since I can't come with example, I like your solution (plus the missing space).

You could use partition instead of the loop to get rid of the loop. Example

val (left, right) = listOf(1,2,3).partition{ it ==2}

You're right. It needs a trailing space. I will add one. Partition is neater and cleaner too.

holgerbrandl · 2017-12-01T00:06:29Z

src/main/kotlin/kscript/app/ResolveIncludes.kt

-            // 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) }


imho should be else if(!it.startsWith("#!/")) ( to get rid of the ugly { } and to streamline the code

holgerbrandl · 2017-12-01T00:23:13Z

Great work! I know it's asking a lot, but could you also create a unit test to test this new feature? Just to make sure that we don't break it in the future.

ilanpillemer · 2017-12-01T00:41:15Z

I'll add a unit test.

ilanpillemer · 2017-12-01T15:03:35Z

@holgerbrandl added unit test and trailing space for the predicate. Is this good now?

holgerbrandl · 2017-12-01T20:40:35Z

Looks great. thanks a lot for your contribution.

holgerbrandl · 2017-12-01T21:13:34Z

I don't think that we can fix it, but it still would break if the user does

val import = 3
import to 2
import + 3
// and any other operator as well.

// or
/**
bla bla
import this
 */

Both is valid kotlin code.

ilanpillemer added 3 commits November 30, 2017 21:49

collects imports, removes bang line, and prepends at top of script

5c2128d

install maven with apt-get rather than sdkman

4dcd616

remove unneccessary code that does nothing

1c5f638

holgerbrandl reviewed Dec 1, 2017

View reviewed changes

ilanpillemer added 6 commits December 1, 2017 14:19

add failing unit test for import without trailing space

67eff8e

change file names so ide does not have falase positive squiqqlies

5ec6ed0

made test pass for import with trailing space only

06b5415

changed way of checking null, switched if else around as per review

e2d1794

added back specific null test, as "i do not exist.kts" test fails

f7d8e02

revert travis.yml change so sdkman now installs maven again (I hope)

caa56e1

holgerbrandl merged commit f759ec4 into kscripting:master Dec 1, 2017

ilanpillemer deleted the consolidate_includes branch December 12, 2017 04:54

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

collects imports, removes bang line, and prepends at top of script #75

collects imports, removes bang line, and prepends at top of script #75

ilanpillemer commented Nov 30, 2017

holgerbrandl Nov 30, 2017

ilanpillemer Dec 1, 2017

ilanpillemer Dec 1, 2017

holgerbrandl Nov 30, 2017

ilanpillemer Dec 1, 2017

holgerbrandl Dec 1, 2017

holgerbrandl Nov 30, 2017

ilanpillemer Dec 1, 2017

ilanpillemer Dec 1, 2017

holgerbrandl Dec 1, 2017

holgerbrandl Nov 30, 2017 •

edited

Loading

ilanpillemer Dec 1, 2017

holgerbrandl Dec 1, 2017

holgerbrandl commented Dec 1, 2017

ilanpillemer commented Dec 1, 2017

ilanpillemer commented Dec 1, 2017

holgerbrandl commented Dec 1, 2017

holgerbrandl commented Dec 1, 2017

collects imports, removes bang line, and prepends at top of script #75

collects imports, removes bang line, and prepends at top of script #75

Conversation

ilanpillemer commented Nov 30, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

holgerbrandl Nov 30, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

holgerbrandl commented Dec 1, 2017

ilanpillemer commented Dec 1, 2017

ilanpillemer commented Dec 1, 2017

holgerbrandl commented Dec 1, 2017

holgerbrandl commented Dec 1, 2017

holgerbrandl Nov 30, 2017 •

edited

Loading