Implement SPICE-0009 External Readers #660
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
HT154
commented
Sep 23, 2024
HT154
force-pushed
the
external-readers
branch
4 times, most recently
from
337ab5a
to
9d8a587
Compare
HT154
force-pushed
the
external-readers
branch
2 times, most recently
from
f6d8810
to
19bc962
Compare
HT154
force-pushed
the
external-readers
branch
9 times, most recently
from
a935e9d
to
9251522
Compare
bioball
requested changes
Oct 14, 2024
There was a problem hiding this comment.
Did a first pass.
This is looking pretty good! Looks pretty polished, with just a couple of issues.
Design question: what do we want to do about env vars or CWD of the process?
What should users expect if thay do:
pkl --env-var foo=bar --external-module-reader ldap=my-ldap-reader
Should my-ldap-reader receive foo=bar as an env var? Or should there be some way to configure the env vars that get passed to this reader?
Same question applies for --working-dir.
Thoughts: probably not appropriate to pass these values into the process, but it should be clearly documented. But, we should probably have some way to configure env var, as well as CWD for these processes.
pkl-commons-cli/src/main/kotlin/org/pkl/commons/cli/commands/BaseOptions.kt
Show resolved
Hide resolved
pkl-core/src/main/java/org/pkl/core/externalProcess/ExternalProcessImpl.java
Outdated
Show resolved
Hide resolved
HT154
force-pushed
the
external-readers
branch
3 times, most recently
from
4fc48da
to
98ec98f
Compare
HT154
force-pushed
the
external-readers
branch
4 times, most recently
from
9323690
to
e561cc3
Compare
bioball
requested changes
Oct 17, 2024
pkl-core/src/main/java/org/pkl/core/module/ModuleKeyFactories.java
Outdated
Show resolved
Hide resolved
pkl-config-java/src/main/java/org/pkl/config/java/ExternalReaderRuntime.java
Outdated
Show resolved
Hide resolved
pkl-core/src/main/java/org/pkl/core/externalReader/package-info.java
Outdated
Show resolved
Hide resolved
pkl-core/src/main/java/org/pkl/core/resource/ResourceReaders.java
Outdated
Show resolved
Hide resolved
bioball
approved these changes
Oct 18, 2024
There was a problem hiding this comment.
Overall LGTM! Great work!
This also needs eyes from @holzensp @stackoverflow because it touches the core language.
HT154
force-pushed
the
external-readers
branch
from
f5ba7c7
to
5d3aa97
Compare
stackoverflow
approved these changes
Oct 24, 2024
pkl-core/src/main/java/org/pkl/core/messaging/AbstractMessagePackEncoder.java
Show resolved
Hide resolved
HT154
force-pushed
the
external-readers
branch
from
e5e2c3e
to
8b3ecdd
Compare
holzensp
approved these changes
Oct 25, 2024
HT154
force-pushed
the
external-readers
branch
2 times, most recently
from
36f535f
to
9972d3d
Compare
This is preparatory work for [SPICE-0009](apple/pkl-evolution#10). It is being contributed in a separate pull request to ease review. The Message, Message(En|De)coder, and MessageTransport types have been ported to Java and moved to a new `org.pkl.core.messaging` package.
[SPICE-0009](apple/pkl-evolution#10) New close flow
Box Bytes, fix error CLI handling during project load Bytes equals compare
Co-authored-by: Daniel Chao <[email protected]> Apply suggestions from code review Co-authored-by: Daniel Chao <[email protected]> Rename --external-* CLI flags to --external-*-reader Rename ExternalProcess -> ExternalReaderProcess Add org.pkl.core.util.Readers.closeQuietly and deprecate ModuleReaders.closeQuietly Clean up doc comments Refactor ExternalReaderRuntime into pkl-config-java Refactor out External*Resolver classes, rename ResourceReaders.ExternalDelegate and ModuleKeys.External to MessageTransport
…al resolvers into module/resource packages
Co-authored-by: Islon Scherer <[email protected]>
HT154
force-pushed
the
external-readers
branch
3 times, most recently
from
280ef78
to
06708b6
Compare
Work around weird kt interop issue
HT154
force-pushed
the
external-readers
branch
from
06708b6
to
e3f777a
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
SPICE-0009
Includes the changes from #640
Adoption of this feature in language binding libraries: