refactor: Deprecate unused actor methods and types #2784
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
didier-wenzek
temporarily deployed
to
Test Pull Request
GitHub Actions
Inactive
Codecov ReportAttention: Patch coverage is
Additional details and impacted files
|
Robot Results
|
didier-wenzek
temporarily deployed
to
Test Pull Request
GitHub Actions
Inactive
didier-wenzek
had a problem deploying
to
Test Pull Request
GitHub Actions
Failure
didier-wenzek
temporarily deployed
to
Test Pull Request
GitHub Actions
Inactive
didier-wenzek
requested review from
albinsuresh,
jarhodes314 and
rina23q
as code owners
albinsuresh
approved these changes
Mar 20, 2024
Comment on lines
183
to
185
| let mut test_box = SimpleMessageBoxBuilder::new("Test box", 16); | ||
| test_box.connect_sink(NoConfig, &builder.connect_client(test_box.get_sender())); | ||
| let mut test_box = test_box.build(); |
There was a problem hiding this comment.
Why not the former variant itself, which is the same but simpler?
Suggested change
| let mut test_box = SimpleMessageBoxBuilder::new("Test box", 16); | |
| test_box.connect_sink(NoConfig, &builder.connect_client(test_box.get_sender())); | |
| let mut test_box = test_box.build(); | |
| let mut test_box = builder.new_client_box(); |
There was a problem hiding this comment.
This is a residue of my attempt to fully remove the new_client_box() test helper.
Reverted.
Comment on lines
289
to
294
| let mut test_box = SimpleMessageBoxBuilder::new("Test box", 16); | ||
| test_box.connect_sink( | ||
| NoConfig, | ||
| &self.box_builder.connect_client(test_box.get_sender()), | ||
| ); | ||
| test_box.build() |
There was a problem hiding this comment.
Same comment as above.
There was a problem hiding this comment.
This is a residue of my attempt to fully remove the new_client_box() test helper.
Reverted.
| @@ -151,6 +156,9 @@ pub trait Service<Request: Message, Response: Message>: | |||
| /// The client provides a `response_sender` on which it wants to response to be sent to. | |||
| /// In exchange the client is returned a request sender on which its requests will have to be sent. | |||
| fn connect_client(&mut self, response_sender: DynSender<Response>) -> DynSender<Request>; | |||
|
|
|||
| /// Create a simple message box connected to a server box under construction. | |||
| fn new_client_box(&mut self) -> SimpleMessageBox<Response, Request>; | |||
There was a problem hiding this comment.
Wondering if this function should this be hidden behind a test feature flag to make the intent clear?
There was a problem hiding this comment.
Hidden behind #[cfg(feature = "test-helpers")]
This method was only used in a test. Also deprecate `WrappedInput` type. Which was adding little compared to Result<Option<Input>, RuntimeRequest>. Signed-off-by: Didier Wenzek <[email protected]>
My plan was to fully deprecate the RuntimeEvent type. However, these events are actually useful to test the runtime. So let this unchanged for now. Signed-off-by: Didier Wenzek <[email protected]>
I'm proud of this Probe abstraction which really demonstrate the flexibility of the actor builder traits. However, this abstraction is used by not even a single test. Aiming to reduce the number of concepts introduce by this crate, I prefer to remove it. Signed-off-by: Didier Wenzek <[email protected]>
The new_client_box method has not been fully deprecated but moved to the Service trait. This makes this trait a bit more contrieved (with 3 ways to connect clients) and will have to be fixed. However, I prefer this temporary situation because moving the method in a trait Ext was only hidding the issue. I will address that in a following step. Signed-off-by: Didier Wenzek <[email protected]>
didier-wenzek
force-pushed
the
refactor/deprecate-unused-actor-methods
branch
from
b144b96
to
e508f11
Compare
didier-wenzek
temporarily deployed
to
Test Pull Request
GitHub Actions
Inactive
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.
Proposed changes
MessageReceiver::recv_message()enum WrappedInputenum RuntimeEvent(from the public API - still used internally)trait ServiceConsumerExtand thewith_probe()test helpertrait ServiceProviderExtand thenew_client_box()test helperTypes of changes
Paste Link to the issue
Checklist
cargo fmtas mentioned in CODING_GUIDELINEScargo clippyas mentioned in CODING_GUIDELINESFurther comments