rust-socketio icon indicating copy to clipboard operation
rust-socketio copied to clipboard

Fix the implementation for the multi payload

Open shenjackyuanjie opened this issue 2 years ago • 5 comments

tested on true workflow

a part of #385

and will fix #409

shenjackyuanjie avatar Jan 24 '24 09:01 shenjackyuanjie

@1c3t3a please take a few minute to view this PR

shenjackyuanjie avatar Feb 01 '24 13:02 shenjackyuanjie

Codecov Report

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

Project coverage is 91.80%. Comparing base (decb053) to head (a4f04d8).

:exclamation: Current head a4f04d8 differs from pull request most recent head 31f0fe3. Consider uploading reports for the commit 31f0fe3 to get more accurate results

Files Patch % Lines
socketio/src/client/raw_client.rs 75.00% 3 Missing :warning:
socketio/src/asynchronous/client/client.rs 71.42% 2 Missing :warning:
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #392      +/-   ##
==========================================
- Coverage   91.80%   91.80%   -0.01%     
==========================================
  Files          36       36              
  Lines        5124     5123       -1     
==========================================
- Hits         4704     4703       -1     
  Misses        420      420              

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

codecov[bot] avatar Mar 30 '24 15:03 codecov[bot]

Hey @shenjackyuanjie, I appreciate your interest in this repository and project. I see you identified a few things that we can improve upon and a few features we can implement, which is great!

In order to keep the code maintainable and healthy, I highly value small and easily reviewable chunks of code (reading PRs). Also every new code must come with sufficient testing and should be of reasonable quality.

For the changes you do to the project, I noticed you don't necessarily follow that approach. For example this PR contains a bunch of different feature implementations. With this it is not possible for me to review that code and for you it reduces the chance of ever merging the feature.

To move forward I advise you to create small increments of high quality code (you know what each line of code you submit does and why it needs to be there) for me to review. Also please break things down in smaller features and add sufficient tests for everything, otherwise I will reject patches.

1c3t3a avatar Mar 30 '24 15:03 1c3t3a

ah, I'm sorry with the last commit, I forget it's still on this PR I will reset the branch back

shenjackyuanjie avatar Mar 30 '24 15:03 shenjackyuanjie

Could you please add a test or further explain what exactly that code enables? :)

the first is just a leftover comment, there aren't any unwrap

I add more comment for the second one

shenjackyuanjie avatar Mar 31 '24 18:03 shenjackyuanjie

@1c3t3a please have a check of the code and the comment

shenjackyuanjie avatar Apr 03 '24 16:04 shenjackyuanjie

Can you please add a test for the behavior you're changing here? :)

1c3t3a avatar Apr 03 '24 20:04 1c3t3a

code coverage faild …… weird, but the test do passed

shenjackyuanjie avatar Apr 10 '24 17:04 shenjackyuanjie

well…… I'll clean them up tonight

shenjackyuanjie avatar Apr 10 '24 22:04 shenjackyuanjie

the code coverage still broke ╮( ̄⊿ ̄)╭

shenjackyuanjie avatar Apr 11 '24 09:04 shenjackyuanjie

@1c3t3a I think you may need a fix for it

shenjackyuanjie avatar Apr 11 '24 12:04 shenjackyuanjie

@1c3t3a I think you may need a fix for it

Oke, it doesn't matter for now, we can fix this separately. Thanks for the PR!

1c3t3a avatar Apr 11 '24 13:04 1c3t3a