Implement shared custom data #428

kitgxrl · 2024-04-29T16:24:32Z

Implements #427

kitgxrl · 2024-04-29T16:42:01Z

Will write tests (if desired) and documentation either when I get home or later tonight!

kitgxrl · 2024-04-29T18:27:04Z

Opening this up for review. I can add tests if they're desired.

codecov · 2024-04-30T07:21:25Z

Codecov Report

Attention: Patch coverage is 21.42857% with 22 lines in your changes are missing coverage. Please review.

Project coverage is 91.49%. Comparing base (fe3c3d8) to head (f27bdbf).
Report is 5 commits behind head on main.

❗ Current head f27bdbf differs from pull request most recent head 0175908. Consider uploading reports for the commit 0175908 to get more accurate results

Files	Patch %	Lines
socketio/src/asynchronous/client/client.rs	12.50%	7 Missing ⚠️
socketio/src/client/raw_client.rs	22.22%	7 Missing ⚠️
socketio/src/asynchronous/client/builder.rs	20.00%	4 Missing ⚠️
socketio/src/client/builder.rs	33.33%	4 Missing ⚠️

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.
📢 Have feedback on the report? Share it here.

1c3t3a

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?

1c3t3a · 2024-05-04T10:14:16Z

socketio/src/asynchronous/client/builder.rs

        }
    }

+    pub fn data<D: std::any::Any + Send + Sync>(mut self, data: Arc<D>) -> Self {


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.

set_data sounds good to me.

1c3t3a · 2024-05-04T10:14:59Z

socketio/src/asynchronous/client/builder.rs

        }
    }

+    pub fn data<D: std::any::Any + Send + Sync>(mut self, data: Arc<D>) -> Self {


Why already take the Arc? Can't we get a fat pointer? Or does that play out really badly with lifetimes?

1c3t3a · 2024-05-04T10:15:26Z

socketio/src/asynchronous/client/builder.rs

        }
    }

+    pub fn data<D: std::any::Any + Send + Sync>(mut self, data: Arc<D>) -> Self {


Please add documentation for the method. IMO this is the perfect place for explaining what data actually is.

Yeah! Sure, I mainly forgot to add documentation here honestly.

1c3t3a · 2024-05-04T10:18:27Z

socketio/src/asynchronous/client/client.rs

+        self.try_data()
+            .expect("Client::data does not match ClientBuilder::data")


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.

Generic parameter was my main concern, sounds like it'd get messy and fast.

1c3t3a · 2024-05-04T10:20:42Z

socketio/src/asynchronous/client/client.rs

+    /// 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>> {


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?

data_unchecked doesn't really sit right with me. Maybe just try_data returning the option is better suited.

Or rather just having a custom_data function that returns the option might be better, not try_data

1c3t3a · 2024-05-04T10:21:20Z

socketio/src/asynchronous/client/client.rs

            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()


Panicing in library code is always a risky thing. We rather want controllable errors (Results).

1c3t3a · 2024-05-04T10:21:51Z

socketio/src/client/raw_client.rs

+    /// 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()
+    }
+


Same comments as above!

kitgxrl · 2024-05-06T23:30:38Z

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 custom_data and shared_data sound good to me, I didn't think about how data could be misleading, thanks for pointing that out. If I think of something better I'll be sure to bring it up!

1c3t3a · 2024-05-07T15:48:32Z

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 custom_data, but feel free to come up with something different.

kitgxrl · 2024-05-15T08:02:55Z

@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 set_data, I'll try and look over this in the next day or two when I have a chance. Apologies for going over this so late haha.

Morb0 · 2024-11-22T21:39:41Z

Any chance?

kitgxrl added 2 commits April 29, 2024 12:20

feat: custom data on sync client

aa7d8a3

feat: custom data on async client

0f3d383

doc: add documentation for data and try_data

f410d7c

kitgxrl marked this pull request as ready for review April 29, 2024 18:27

kitgxrl changed the title ~~WIP: Implement shared custom data~~ Implement shared custom data Apr 29, 2024

doc: fix typos

f27bdbf

1c3t3a reviewed May 4, 2024

View reviewed changes

kitgxrl added 5 commits May 15, 2024 03:38

refactor: rename data to set_data on client bulder

220bdbe

doc: set temporary docs for set_data

101193d

feat: remove data functions that panic

fd8012b

refactor: rename try_data to custom_data

d18cb8f

doc: reference ClientBuilder::set_data instead

0175908

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement shared custom data #428

Implement shared custom data #428

kitgxrl commented Apr 29, 2024

kitgxrl commented Apr 29, 2024 •

edited

Loading

kitgxrl commented Apr 29, 2024

codecov bot commented Apr 30, 2024 •

edited

Loading

1c3t3a left a comment

1c3t3a May 4, 2024

kitgxrl May 15, 2024

1c3t3a May 4, 2024

1c3t3a May 4, 2024

kitgxrl May 15, 2024

1c3t3a May 4, 2024

kitgxrl May 15, 2024

1c3t3a May 4, 2024

kitgxrl May 15, 2024 •

edited

Loading

kitgxrl May 15, 2024 •

edited

Loading

1c3t3a May 4, 2024

1c3t3a May 4, 2024

kitgxrl commented May 6, 2024

1c3t3a commented May 7, 2024

kitgxrl commented May 15, 2024

Morb0 commented Nov 22, 2024

		self.try_data()
		.expect("Client::data does not match ClientBuilder::data")

Implement shared custom data #428

Are you sure you want to change the base?

Implement shared custom data #428

Conversation

kitgxrl commented Apr 29, 2024

kitgxrl commented Apr 29, 2024 • edited Loading

kitgxrl commented Apr 29, 2024

codecov bot commented Apr 30, 2024 • edited Loading

Codecov Report

1c3t3a left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kitgxrl May 15, 2024 • edited Loading

Choose a reason for hiding this comment

kitgxrl May 15, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kitgxrl commented May 6, 2024

1c3t3a commented May 7, 2024

kitgxrl commented May 15, 2024

Morb0 commented Nov 22, 2024

kitgxrl commented Apr 29, 2024 •

edited

Loading

codecov bot commented Apr 30, 2024 •

edited

Loading

kitgxrl May 15, 2024 •

edited

Loading

kitgxrl May 15, 2024 •

edited

Loading