Implement shared custom data #428
Conversation
|
Will write tests (if desired) and documentation either when I get home or later tonight! |
|
Opening this up for review. I can add tests if they're desired. |
kitgxrl
changed the title
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #428 +/- ##
==========================================
- Coverage 91.86% 91.49% -0.38%
==========================================
Files 36 36
Lines 5166 5194 +28
==========================================
+ Hits 4746 4752 +6
- Misses 420 442 +22 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Hey @kitgxrl, first of all thanks a lot for the PR! I really like the idea. I added some comments to the code on a first pass.
Also I would like to discuss the naming: I think data is a bit misleading. Reading this I would think I get internal connection data of the protocol/client. Instead it'd be better to have a name that directly implies that all control of that "data" is with the user. So e.g. shared_data, custom_data or sth along these lines (I didn't think too long about this). What's your opinion?
| } | ||
| } | ||
|
|
||
| pub fn data<D: std::any::Any + Send + Sync>(mut self, data: Arc<D>) -> Self { |
There was a problem hiding this comment.
nit: Maybe name set_data? Didn't find that this is forbidden according to the naming guidelines this crate uses: https://rust-lang.github.io/api-guidelines/naming.html. Only getters omit the prefix.
| } | ||
| } | ||
|
|
||
| pub fn data<D: std::any::Any + Send + Sync>(mut self, data: Arc<D>) -> Self { |
There was a problem hiding this comment.
Why already take the Arc? Can't we get a fat pointer? Or does that play out really badly with lifetimes?
| } | ||
| } | ||
|
|
||
| pub fn data<D: std::any::Any + Send + Sync>(mut self, data: Arc<D>) -> Self { |
There was a problem hiding this comment.
Please add documentation for the method. IMO this is the perfect place for explaining what data actually is.
There was a problem hiding this comment.
Yeah! Sure, I mainly forgot to add documentation here honestly.
| self.try_data() | ||
| .expect("Client::data does not match ClientBuilder::data") |
There was a problem hiding this comment.
Mhmmm... I don't really like that we need a runtime check here... Wdyt, would it be possible to make this a generic parameter of both the Client and the builder? If this turns out messy, feel free to omit.
There was a problem hiding this comment.
Generic parameter was my main concern, sounds like it'd get messy and fast.
| /// Attempts to fetch data given by [`ClientBuilder::data`] | ||
| /// | ||
| /// None is returned if data was not given or data does not match [`ClientBuilder::data`] | ||
| pub fn try_data<D: Send + Sync + 'static>(&self) -> Option<Arc<D>> { |
There was a problem hiding this comment.
Mhm, I don't really like the naming here. Usually a try_ prefix implies a Result, which we don't have here. How about we name this method data (returning an option) and the other one data_unchecked which panics? Or we actually don't provide a panic method (because that's usually a bad idea anyways, no library should panic), and just make the upper one return a Result?
There was a problem hiding this comment.
data_unchecked doesn't really sit right with me. Maybe just try_data returning the option is better suited.
There was a problem hiding this comment.
Or rather just having a custom_data function that returns the option might be better, not try_data
| builder: Arc::new(RwLock::new(builder)), | ||
| disconnect_reason: Arc::new(RwLock::new(DisconnectReason::default())), | ||
| }) | ||
| } | ||
|
|
||
| /// Fetches data given by [`ClientBuilder::data`] | ||
| pub fn data<D: Send + Sync + 'static>(&self) -> Arc<D> { | ||
| self.try_data() |
There was a problem hiding this comment.
Panicing in library code is always a risky thing. We rather want controllable errors (Results).
socketio/src/client/raw_client.rs
Outdated
| /// Fetches data given by [`ClientBuilder::data`] | ||
| pub fn data<D: Send + Sync + 'static>(&self) -> Arc<D> { | ||
| self.try_data() | ||
| .expect("RawClient::data does not match ClientBuilder::data") | ||
| } | ||
|
|
||
| /// Attempts to fetch data given by [`ClientBuilder::data`] | ||
| /// | ||
| /// None is returned if data was not given or data does not match [`ClientBuilder::data`] | ||
| pub fn try_data<D: Send + Sync + 'static>(&self) -> Option<Arc<D>> { | ||
| Arc::clone(&self.data).downcast().ok() | ||
| } | ||
|
|
|
Hey @1c3t3a thanks for the review! I'll take a look at the comments made sometime Thursday or Friday due to exams over the next two days (I apologize for the late response). Both |
|
No worries, there is absolutely now need to apologize! I am super happy you work on this in your free time :) Good luck with the exams! And in that case I'd personally prefer |
|
@1c3t3a Took a look at most of what you said, let me know if I missed anything big you'd like to go over. Only thing I believe I haven't looked into yet is using generics on Client and ClientBuilder and accepting a fat pointer on |
Implements #427