Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix the implementation for the multi payload #392

Merged
merged 9 commits into from
Apr 11, 2024

Conversation

shenjackyuanjie
Copy link
Contributor

@shenjackyuanjie shenjackyuanjie commented Jan 24, 2024

tested on true workflow

a part of #385

and will fix #409

@shenjackyuanjie
Copy link
Contributor Author

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

socketio/src/client/raw_client.rs Outdated Show resolved Hide resolved
socketio/src/client/raw_client.rs Outdated Show resolved Hide resolved
Copy link

codecov bot commented Mar 30, 2024

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

❗ 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 ⚠️
socketio/src/asynchronous/client/client.rs 71.42% 2 Missing ⚠️
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              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@1c3t3a
Copy link
Owner

1c3t3a commented Mar 30, 2024

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.

@shenjackyuanjie
Copy link
Contributor Author

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

@shenjackyuanjie shenjackyuanjie force-pushed the mult_payload branch 4 times, most recently from 23138e4 to f201a4b Compare March 31, 2024 13:05
@shenjackyuanjie shenjackyuanjie changed the title implment multi payload Fix the implementation for the multi payload Mar 31, 2024
Copy link
Owner

@1c3t3a 1c3t3a left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

socketio/src/asynchronous/client/client.rs Outdated Show resolved Hide resolved
socketio/src/asynchronous/client/client.rs Outdated Show resolved Hide resolved
@shenjackyuanjie
Copy link
Contributor Author

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
Copy link
Contributor Author

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

@1c3t3a
Copy link
Owner

1c3t3a commented Apr 3, 2024

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

@shenjackyuanjie
Copy link
Contributor Author

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

Copy link
Owner

@1c3t3a 1c3t3a left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The CI seems like a flake, lets work in your changes and hope the action is fixed by then.

engineio/src/packet.rs Show resolved Hide resolved
socketio/src/client/raw_client.rs Outdated Show resolved Hide resolved
socketio/src/client/raw_client.rs Outdated Show resolved Hide resolved
@shenjackyuanjie
Copy link
Contributor Author

well……
I'll clean them up tonight

@shenjackyuanjie
Copy link
Contributor Author

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

@shenjackyuanjie
Copy link
Contributor Author

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

@1c3t3a
Copy link
Owner

1c3t3a commented Apr 11, 2024

@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 1c3t3a merged commit 2636b23 into 1c3t3a:main Apr 11, 2024
3 of 4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Multiple payload still broke
2 participants