feature: Add InMemoryModuleReader for evaluating virtual Pkl modules from a Swift dictionary#139
Conversation
…from a Swift dictionary
HT154
left a comment
There was a problem hiding this comment.
I'm a little hesitant to add this directly to the PklSwift module as it takes pretty opinionated approach to filesystem emulation. Unlike pkl-go (where we have fsReader), Swift provides no standard library filesystem abstraction.
I think I'd be more inclined to add this in a new PklSwiftContrib framework target that clients can adopt selectively, especially since testing is a large portion of this API's use case.
I'm interested in @bioball's thoughts on this as well.
| /// The URI-keyed dictionary of Pkl source strings this reader serves. | ||
| /// | ||
| /// Keys are fully-qualified URI strings (e.g. `"mem:config.pkl"` or | ||
| /// `"mem:/catalog/swallow.pkl"`). Values are the raw Pkl source text. | ||
| public let modules: [String: String] |
There was a problem hiding this comment.
Primary concern: there's next to nothing preventing a user from holding this wrong.
I think it's a little more usable if you don't require the scheme in the keys, and I think most of my concern would be addressed if the initializer checked each key to ensure it starts with /.
There was a problem hiding this comment.
Primary concern: there's next to nothing preventing a user from holding this wrong.
I think it's a little more usable if you don't require the scheme in the keys, and I think most of my concern would be addressed if the initializer checked each key to ensure it starts with
/.
A separate framework target keeps the core PklSwift module lean and lets clients opt in selectively. I'll move InMemoryModuleReader into a new PklSwiftContrib target
| public func read(url: URL) async throws -> String { | ||
| let key = url.absoluteString | ||
| guard let source = modules[key] else { | ||
| throw InMemoryModuleReaderError.moduleNotFound(key) | ||
| } | ||
| return source | ||
| } |
There was a problem hiding this comment.
Heads up that this may need to change due to #127
There was a problem hiding this comment.
Heads up that this may need to change due to #127
I took a look it changes ResourceReader.read to return an optional, but doesn't touch ModuleReader, so the current implementation shouldn't conflict directly.
| /// common `uri` prefix, and groups results by their first remaining path component. | ||
| /// Directory entries (intermediate path segments) are synthesised automatically. | ||
| /// Output is sorted by name for deterministic glob results. | ||
| public func listElements(uri: URL) async throws -> [PathElement] { |
There was a problem hiding this comment.
This isn't really how this method is supposed to work; readers are either hierarchical (/ denotes a path segment), or opaque (/ does not mean anything special).
Hierarchical URIs are like file:, http:, where the path portion denotes some structure (/foo/bar is the parent of /foo/bar/baz). Opaque URIs are like prop: and env:, where the path portion is just a raw value.
In the case of hierarchical URIs, the path portion must start with /.
In the case of non-hierarchical URIs, the listElements request sends over dummy as the path, because the reader just needs to return everything (e.g. the env reader receives env:dummy).
One option here could be to provide an "in-memory filesystem" type of API, something like:
struct VirtualFileReader: ModuleReader {
public let scheme: String
public let files: [String: String]
public init(_ scheme: String, _ files: [String: String]) throws {
// logic to require all files start with `/`
}
}I think it might also make sense to wait until there's a file system API in the Swift stdlib before introducing an API like this (see https://forums.swift.org/t/pitch-2-add-filepath-to-stdlib/85695).
Adds InMemoryModuleReader, a built-in ModuleReader backed by a [String: String] dictionary mapping URI strings to Pkl source text. This enables evaluating multi-file virtual Pkl configurations without touching disk — useful for unit testing and assembling configs dynamically at runtime.
Usage:
@HT154 wanted your opinion on this update