This repository has been archived by the owner on Dec 12, 2024. It is now read-only.
-
Notifications
You must be signed in to change notification settings - Fork 2
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
04fc794
to
ad3da5d
Compare
451b5e9
to
4dc6dbb
Compare
Fixes #13 |
And modify its return type to be an enum instead of Box
cmpadden
approved these changes
Dec 12, 2024
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you @christeefy -- very appreciative that you took the time to outline the decision in the PR.
I'm going to merge this for now, as I plan to move this code base to the community-integration
repo soon.
Side note, I plan on persisting the Git history in migration, so attribution will not be lost.
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
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.
Migrate
lib.rs
'sPipesFileMessageWriter
implementation intoMessageWriter
andMessageWriterChannel
traits, following thePipesMessageWriter
andPipesMessageWriterChannel
Python base classes.Note
MessageWriter::open
differs from the Python'sPipesMessageWriter.open
implementation. Rust has no simple way to yield (afaik). Python's implementation yields so that it can perform cleanup. Instead, in the Rust implementation, we delegate the cleanup responsibility to theDrop
trait — loosely analogous to a C destructor.This is a simplified implementation that ignores lower-level
LogWriter
andLogWriterChannel
abstractions in the Python implementation, to prevent this PR from getting larger than it already is.This PR ended up using two advanced Rust functionalities, so I've included write-ups and inline documentation for posterity.
1.
MessageWriter
andMessageWriterChannel
as associated typesAfter a few attempts of implementing it in Rust (and with help from ChatGPT), I've settled on using Associated Types:
The original reason for me doing this is to limit the "blast radius" caused by the
PipesDefaultMessageWriter
struct.That struct returns different types of
MessageWriterChannel
s, decided at runtime (see Python implementation). In Rust, every type must be known at compile-time (allowing for static dispatch and optimizations), otherwise they need to be wrapped inBox
(a smart pointer) for dynamic dispatch which incurs a runtime cost.Crucially,
PipesDefaultMessageWriter
's implementation would alterMessageWriter
's trait signature:forcing all future (community)
MessageWriter
implementations to incur this type complexity (needing to understandBox
and dynamic dispatch) & performance penalty.By using associated types, each concrete
MessageWriter
implementation is free to return whateverChannel
it needs without affecting other implementations, so long as thatChannel
implements theMessageWriterChannel
type:Additional Benefits
Using associated types also led to other unexpected benefits:
MessageWriter
implementation must have a corresponding and specificMessageWriterChannel
implementation. In this PR,PipesDefaultMessageWriter
must be used withBox<dyn MessageWriterChannel>
andBox<dyn MessageWriterChannel>
only, even if otherMessageWriterChannel
implementations exist1.PipesContext
with a specificMessageWriter
, the correct and correspondingMessageWriterChannel
is used:LogWriter
andLogWriterChannel
in the future2. Sealing trait methods
The
PipesMessageWriter.get_opened_payload
abstract method in Python is denoted with@final
, preventing overrides. Stable Rust doesn't have a#[final]
attribute yet, and so we'll have to manually seal that method.This is done using a
private::Token
that's only accessible within that module. Callers outside that module cannot access this dummy struct, and therefore cannot override its implementation!However, external callers also cannot call the function without
private::Token
, so a public function is provided.User-facing documentation is also included in the implementation's documentation!
![image](https://private-user-images.githubusercontent.com/8013575/393401137-75e60f89-2b36-4d61-a9ca-fa0633660ae3.png?jwt=eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJpc3MiOiJnaXRodWIuY29tIiwiYXVkIjoicmF3LmdpdGh1YnVzZXJjb250ZW50LmNvbSIsImtleSI6ImtleTUiLCJleHAiOjE3MzkyMzMyNjQsIm5iZiI6MTczOTIzMjk2NCwicGF0aCI6Ii84MDEzNTc1LzM5MzQwMTEzNy03NWU2MGY4OS0yYjM2LTRkNjEtYTljYS1mYTA2MzM2NjBhZTMucG5nP1gtQW16LUFsZ29yaXRobT1BV1M0LUhNQUMtU0hBMjU2JlgtQW16LUNyZWRlbnRpYWw9QUtJQVZDT0RZTFNBNTNQUUs0WkElMkYyMDI1MDIxMSUyRnVzLWVhc3QtMSUyRnMzJTJGYXdzNF9yZXF1ZXN0JlgtQW16LURhdGU9MjAyNTAyMTFUMDAxNjA0WiZYLUFtei1FeHBpcmVzPTMwMCZYLUFtei1TaWduYXR1cmU9OWIyNTE4YzFkYzU0MjI3NDgyMzQzOTNhMmExMDc0MWQyNWMzYWNjNDc0NWVlMGViODk4ODgzZmJiMTMxMzZmYiZYLUFtei1TaWduZWRIZWFkZXJzPWhvc3QifQ.1T7y73yoyv0sqzLJzllKS3psZopE56k8DKo5cbCVYNo)
TODOs
Footnotes
For a generalized version of this, there's Generic Associated Types (GAT) ↩