8000 Refine external reader API by HT154 · Pull Request #762 · apple/pkl · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Refine external reader API #762

Merged
merged 1 commit into from
Oct 31, 2024
Merged

Refine external reader API #762

merged 1 commit into from
Oct 31, 2024

Conversation

HT154
Copy link
Contributor
@HT154 HT154 commented Oct 31, 2024

Thanks to @translatenix for identifying these in #759. This is the subset of items that were either low-hanging fruit or definitely need be addressed before this becomes public API.

  • Encapsulate message transport by removing ExternalReaderProcess.getTransport and adding getModuleResolver and getResourceResolver methods
  • Reuse Random instances within ExternalReaderProcessImpl and module/resource resolvers
  • Externalize all ExternalReaderProcessException messages
  • Add some missing doc comments to ModuleKeyFactories and ResourceReaders methods for external readers
  • Move org.pkl.core.util.Readers to org.pkl.core.Readers

@HT154
Refine external reader API
b117011 
* Encapsulate message transport by removing `ExternalReaderProcess.getTransport` and adding `getModuleResolver` and `getResourceResolver` methods
* Reuse `Random` instances within `ExternalReaderProcessImpl` and module/resource resolvers
* Externalize all `ExternalReaderProcessException` messages
* Add some missing doc comments to `ModuleKeyFactories` and `ResourceReaders` methods for external readers
* Move org.pkl.core.util.Readers to org.pkl.core.Readers
bioball
bioball approved these changes Oct 31, 2024
View reviewed changes
Copy link
Contributor
@bioball bioball left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, good cleanups!

@bioball bioball merged commit e217cfc into apple:main Oct 31, 2024
5 checks passed
@odenix
Copy link
Contributor
odenix commented Nov 1, 2024 •
edited
Loading

@HT154 Did you consider addressing this one?

  • org.pkl.core.module.ResourceReaders API is (too) tightly coupled to reader protocol via ModuleReaderSpec

Perhaps I'm missing something, but a high-level user-facing API that exports a low-level internal API is a grave breach of modularity.

One more idea: Readers.closeQuietly() is totally generic, so it might as well be Closeables.closeQuietly(). Perhaps it's needed somewhere else in the future.

Last but not least, I thought about renaming ExternalReaderProcess to ExternalReader, as the API doesn't seem to mandate a process.

@HT154 HT154 deleted the external-reader-api-refinement branch November 1, 2024 04:21
@HT154
Copy link
Contributor Author
HT154 commented Nov 1, 2024

@translatenix Yup, needed some more time to chew on these

Did you consider addressing this one?

  • org.pkl.core.module.ResourceReaders API is (too) tightly coupled to reader protocol via ModuleReaderSpec

Perhaps I'm missing something, but a high-level user-facing API that exports a low-level internal API is a grave breach of modularity.

The only way I can think of to do this is to move ModuleKeys.ExternalResolver, ModuleKeyFactories.ExternalProcess, ResourceReaders.ExternalResolver, and ResourceReaders.ExternalProcess to org.pkl.core.messaging. Some early feedback on the original PR requested moving these into their current locations. I'm definitely open to doing this, but I'm interested in hearing from @bioball and the crew before I jump into it.

One more idea: Readers.closeQuietly() is totally generic, so it might as well be Closeables.closeQuietly(). Perhaps it's needed somewhere else in the future.

I don't feel too strongly about this, but happy to take it on if there's desire from the core team.

Last but not least, I thought about renaming ExternalReaderProcess to ExternalReader, as the API doesn't seem to mandate a process.

I am planning on opening a separate PR for this tomorrow.

@bioball
Copy link
Contributor
bioball commented Nov 1, 2024 •
edited
Loading

One way to hide types from the messaging package is to define ModuleReaderSpec as an interface in externalreader, and use that as the return type of org.pkl.core.externalreader.ExternalReaderProcess#getModuleReaderSpec.

Then, have record org.pkl.core.messaging.Messages.ModuleReaderSpec implement that interface. And ExternalReaderProcessImpl would just return an instance of that record.

This refactor sounds reasonable, so, go for it @HT154.

I don't feel too strongly about this, but happy to take it on if there's desire from the core team.

Yeah Closeables sounds like a better name.

Last but not least, I thought about renaming ExternalReaderProcess to ExternalReader, as the API doesn't seem to mandate a process.

I'm going to zag here. It's meant for external spawning processes, so, I actually think this is a good name.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bioball bioball bioball approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@HT154 @odenix @bioball
0