apple / pkl-pantry

Shared Pkl packages
Apache License 2.0
229 stars 31 forks source link

[pkl.lua] New package #53

Closed lilyball closed 2 months ago

lilyball commented 2 months ago

pkl.lua is a package with one module, pkl.lua.lua, which provides a ValueRenderer subclass that serializes to Lua.

lilyball commented 2 months ago

Is there any way to test pkldoc generation before publishing? I can't seem to get the command-line version working locally.

bioball commented 2 months ago

Is there any way to test pkldoc generation before publishing? I can't seem to get the command-line version working locally.

This is a little roundabout, but, you can add a quick Gradle configuration to generate pkldoc. You'll need to create a fake doc-package-info.pkl file to get everything to work.

Apply this diff:

diff --git a/build.gradle.kts b/build.gradle.kts
index 688a1e9..1836ce8 100644
--- a/build.gradle.kts
+++ b/build.gradle.kts
@@ -97,6 +97,14 @@ pkl {
             }
         }
     }
+    pkldocGenerators {
+        register("makeLuaPreview") {
+            sourceModules = fileTree("packages/pkl.lua") {
+                include("*.pkl")
+            }
+            outputDir = layout.buildDirectory.dir("pkldoc")
+        }
+    }
 }

 val resolveProjects = tasks.named("resolveProjects") {
diff --git a/packages/pkl.lua/doc-package-info.pkl b/packages/pkl.lua/doc-package-info.pkl
index e69de29..444c362 100644
--- a/packages/pkl.lua/doc-package-info.pkl
+++ b/packages/pkl.lua/doc-package-info.pkl
@@ -0,0 +1,10 @@
+/// A [Lua](https://www.lua.org) renderer.
+amends "pkl:DocPackageInfo"
+
+importUri = "package://pkg.pkl-lang.org/pkl/pkl-pantry/pkl.lua@1.0.0#/"
+
+name = "pkl.lua"
+
+version = "1.0.0"
+
+issueTracker = "https://github.com/"

Then run ./gradlew makeLuaPreview. Then open build/pkldoc/index.html to see the results.

lilyball commented 2 months ago

Thanks that worked great! Though I also had to bump the pkl version in gradle/libs.versions.toml to get some documentation render fixes.

lilyball commented 2 months ago

I have applied all suggestions, with the sole exception that where you recommended doing

if (condition) value
else
  …

I left it as

if (condition) value else
  …

One of your suggestions actually used the latter style, and there are a few other instances of this style in the repository, and the style guide does not cover this case, so I figured it was fine.

I also fixed the indentation in a couple other spots that were not mentioned.

Curiously, moving Prop() and Key() out of the class definition required me to add the const modifier and I don't know why that would be necessary.

bioball commented 2 months ago

One of your suggestions actually used the latter style, and there are a few other instances of this style in the repository, and the style guide does not cover this case, so I figured it was fine.

I guess you're right that we don't cover this. But, I find this much easier to read, because it's clearer that it belongs to the else block. Nevertheless, I won't block your PR on this.

if (condition) value
else
  …

Curiously, moving Prop() and Key() out of the class definition required me to add the const modifier and I don't know why that would be necessary.

This is because class bodies can only reference outside const members. Otherwise, an amended module can possibly change the class definition.

Consider:

// base.pkl
bar = "bar"

function foo() = bar

class MyClass {
  myProp = foo()
}

Then:

amends "base.pkl"

bar = "barbar"
lilyball commented 2 months ago

Ahh right, I was thinking here that the function definition itself couldn't be replaced, but I didn't think about the function being able to reference other non-const values from the module.