Add ResourceReaderFactory abstraction #647
Conversation
| /** | ||
| * Returns a {@link ResourceReader} for the given absolute normalized URI, or {@code | ||
| * Optional.empty()} if this factory cannot handle the given URI. | ||
| * | ||
| * <p>Implementations must not perform any I/O related to the given URI. For example, they must | ||
| * not check if the module represented by the given URI exists. | ||
| * | ||
| * <p>Throws {@link URISyntaxException} if the given URI has invalid syntax. | ||
| * | ||
| * @param uri an absolute normalized URI | ||
| * @return a resource for the given URI | ||
| */ | ||
| Optional<ResourceReader> create(URI uri); |
There was a problem hiding this comment.
For implementing external readers, this method will be the point at which the child process is spawned (if it is not already running) and the resource/module readers it implements will be discovered. When the process needs to be spawned, this call will incur that one-time delay.
This is preparatory work for [SPICE-0009](apple/pkl-evolution#10). It is being contributed in a separate pull request to ease review. This is required to allow deferred launch of external reader processes without requiring up-front configuration of scheme->process mappings. More details on why this is required here: apple/pkl-evolution#10 (comment) One concern here (which applies before this change as well) is that the semantics of module key factories and resource reader factories is reversed. For module key factories, the first factory that answers for a URI scheme "wins", while for resource reader factories, the _last_ reader factory that answers for a URI scheme "wins". This PR preserves that behavior, but it may be worth reconsidering this design at this time. This behavior can be observed in practice in <code>ResourceReadersEvaluatorTest.\`module path\`</code> where `ResourceReaders.modulePath` is added after the pre-configured `ResourceReaders.classPath` and takes precedence. This is a fairly large breaking API change, but in practice most clients will only need to replace a few lines in their calls to `EvaluatorBuilder`, eg. ```java .addResourceReader(ResourceReaders.environmentVariable()) // becomes .addResourceReaderFactory(ResourceReaderFactories.environmentVariable()) ```
HT154
force-pushed
the
resource-reader-factory
branch
from
0f9f31a
to
bbb67e2
Compare
Ah, that's pretty unfortunate... that would be a pretty surprising change of behavior, for interfaces that otherwise look so similar. It'd probably be better to flip the order for resource readers, at the cost of a bigger breaking change here. I'm am wondering if we do want the factory approach here. Do we need external module/resource readers to have unknown schemes ahead of time? Let's chat more about it on the spice. |
This is preparatory work for SPICE-0009. It is being contributed in a separate pull request to ease review.
This is required to allow deferred launch of external reader processes without requiring up-front configuration of scheme->process mappings. More details on why this is required here: apple/pkl-evolution#10 (comment)
One concern here (which applies before this change as well) is that the semantics of module key factories and resource reader factories is reversed. For module key factories, the first factory that answers for a URI scheme "wins", while for resource reader factories, the last reader factory that answers for a URI scheme "wins". This PR preserves that behavior, but it may be worth reconsidering this design at this time.
This behavior can be observed in practice in
ResourceReadersEvaluatorTest.`module path`whereResourceReaders.modulePathis added after the pre-configuredResourceReaders.classPathand takes precedence.This is a fairly large breaking API change, but in practice most clients will only need to replace a few lines in their calls to
EvaluatorBuilder, eg.