Untangle external reader code #776
Conversation
- move the following classes into package externalreader: - ExternalModuleResolver - ExternalResourceResolver - MessageTransportModuleResolver (renamed to ExternalModuleResolverImpl, made package-private) - MessageTransportResourceResolver (renamed to ExternalResourceResolverImpl, made package-private) - replace interface ExternalModuleResolver.Spec with record ExternalModuleReaderSpec - replace interface ExternalResourceResolver.Spec with record ExternalResourceReaderSpec - translate between messaging.ResourceReaderSpec and ExternalResourceReaderSpec (eliminates dependency from messaging on higher layer) - translate between messaging.ResourceResolverSpec and ExternalResourceResolverSpec (eliminates dependency from messaging on higher layer) - add ServerMessages.ExternalReader and translate between this message component and the PklEvaluatorSettings.ExternalReader API - add ServerMessages.Proxy and translate between this message component and the PklEvaluatorSettings.Proxy API - change type of CreateEvaluatorRequest.allowedModules/allowedResources from List<Pattern>? to List<String>? - removes a lot of code - should not need to create a Pattern object to send a message - remove public method evaluatorSettings.PklEvaluatorSettings.Proxy.create() - only seems useful internally, inlined
- class names were too long/repetitive - package name provides sufficient context
| import org.pkl.core.evaluatorSettings.PklEvaluatorSettings | ||
| import org.pkl.core.externalreader.ModuleReaderSpec | ||
| import org.pkl.core.externalreader.ReaderProcess | ||
| import org.pkl.core.externalreader.ResourceReaderSpec |
There was a problem hiding this comment.
Given that the resolver types are shared between the client readers and external readers, is org.pkl.core.externalreader really the right place for these types?
There was a problem hiding this comment.
org.pkl.server depending on externalreader seems acceptable to me. These types couldn't stay in module/resource because that creates package cycles and is a layering violation: module/resource already export packagereader.ReaderProcess, hence packagereader shouldn't also depend on them.
pkl-core has serious modularity issues. I just tried to do some damage control in the few hours I had. I think I made a step in the right direction; perhaps you guys will find further improvements.
|
Thanks for the PR, and appreciate the help straightening this out! FYI--this is a little late to get into the 0.27 release. However, we can get this into the main branch once this release is out. We can also take some time to better look at the API of pkl-core and have a clear definition of what's public and what's not (and perhaps use JPMS). |
Just keep in mind that this PR makes breaking changes to APIs introduced in 0.27.
I'd appreciate this. It will likely take quite a while until pkl-core is ready for JPMS. But we can take one step at a time: define which packages are public, enforce at build time that they don't export private packages, etc. |
|
For the next release, could we create a release branch at least one week before the release, and merge only bugfixes from then on? |
|
Yeah, we should definitely add some more process here, including cut-off for features. We'll figure out what that should look like after we get 0.27 out and into the world. Apologies for the lack of structure right now. |
See commit messages for details. Second commit is a pure rename refactoring.