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.

Sign up for GitHub

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

Jump to bottom

Add async_any_callback and async_callback macro with it's docs #399

Merged
merged 5 commits into from
Mar 31, 2024
Merged

Add async_any_callback and async_callback macro with it's docs #399

merged 5 commits into from
Mar 31, 2024

Conversation

shenjackyuanjie
Copy link
Contributor

@shenjackyuanjie shenjackyuanjie commented Mar 15, 2024 •
edited
Loading

Do #395

@shenjackyuanjie shenjackyuanjie mentioned this pull request Mar 15, 2024
Closed
1c3t3a
1c3t3a reviewed Mar 15, 2024
View reviewed changes
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.

LGTM % comments. Thanks a lot for this!

CHANGELOG.md Outdated Show resolved Hide resolved
@codecov Codecov
Copy link

codecov bot commented Mar 15, 2024 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 91.82%. Comparing base (7568572) to head (5a08f98).
Report is 1 commits behind head on main.

Additional details and impacted files 
@@            Coverage Diff             @@
##             main     #399      +/-   ##
==========================================
- Coverage   91.95%   91.82%   -0.14%     
==========================================
  Files          36       36              
  Lines        5073     5124      +51     
==========================================
+ Hits         4665     4705      +40     
- Misses        408      419      +11

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

@1c3t3a
Copy link
Owner

1c3t3a commented Mar 15, 2024

Seems like there is an error with formatting, please also fix that :)

For the failing test I will investigate what is going on.

@shenjackyuanjie
Copy link
Contributor Author

I got it, it's from tokio's feature
I'll just add it and bump the version

@shenjackyuanjie
Copy link
Contributor Author

good……
so now is the doc test issue

@1c3t3a
Copy link
Owner

1c3t3a commented Mar 21, 2024

I fixed the tokio CI issue in main, feel free to rebase :)

@1c3t3a
Copy link
Owner

1c3t3a commented Mar 21, 2024

Just need to fix the doctests and we're good to go! :)

@1c3t3a
Copy link
Owner

1c3t3a commented Mar 30, 2024

It would be great to get this into the new version! If the three comments I posted are resolved and all tests pass I am happy to merge.

@shenjackyuanjie
Copy link
Contributor Author

@1c3t3a please approval the test, I think it will do it

@shenjackyuanjie
Copy link
Contributor Author

hmmm, A quick fix

@shenjackyuanjie
Copy link
Contributor Author

shenjackyuanjie commented Mar 30, 2024 •
edited
Loading

19 min "quick fix"
Just try this pls @1c3t3a

@1c3t3a
Copy link
Owner

1c3t3a commented Mar 30, 2024

I think this won't work either, the issue is as I pointed out, that we're missing an async main function. Please take a look at my comment regarding wrapping the code in the comment.

@shenjackyuanjie
Copy link
Contributor Author

shenjackyuanjie commented Mar 30, 2024 •
edited
Loading

I think this won't work either, the issue is as I pointed out, that we're missing an async main function. Please take a look at my comment regarding wrapping the code in the comment.

oh, sorry about that, mybad

and it's my bad again
the macro didn't contains a use future_util::FutureExt;

@shenjackyuanjie shenjackyuanjie force-pushed the callback_macro branch 2 times, most recently from c35ee3c to 7e95f2e Compare March 30, 2024 18:23
@shenjackyuanjie
Copy link
Contributor Author

what……
fine……

@shenjackyuanjie
add re-export for the macro, and let the test pass
9f07ae7 
just use it from the root
add use futures_util::FutureExt;
@shenjackyuanjie
Copy link
Contributor Author

image

I have tested it locally, looking good

@shenjackyuanjie
Copy link
Contributor Author

@1c3t3a one last time today, please approve the test

@shenjackyuanjie
Copy link
Contributor Author

finally！

today is a good day~

@1c3t3a
Copy link
Owner

1c3t3a commented Mar 31, 2024

Great! Finally passed :) Could you please remove the change of base64 version please? Then we're ready to merge!

1c3t3a
1c3t3a reviewed Mar 31, 2024
View reviewed changes
socketio/Cargo.toml Outdated Show resolved Hide resolved
@shenjackyuanjie
Copy link
Contributor Author

@1c3t3a there you go~

1c3t3a
1c3t3a approved these changes Mar 31, 2024
View reviewed changes
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.

LGTM, thank you very much! :)

@1c3t3a 1c3t3a merged commit 7670f7e into 1c3t3a:main Mar 31, 2024
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@1c3t3a 1c3t3a 1c3t3a approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@shenjackyuanjie @1c3t3a