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

Be able to pass in closure to advertise_service #142

Merged
merged 6 commits into from
Oct 24, 2023

Conversation

entire
Copy link
Contributor

@entire entire commented Oct 21, 2023

Description

  • Would like to be able to pass in closures to advertise_service
  • By using a generic type F for the server function argument in advertise_service this allows us to pass in more than just functions, i.e. closures,
  • Would like to be able to pass in closures so that one can pass data through the closure to the callback that is executed by advertise_service
  • Updated the example code as well to show how it would work

Fixes

Issue Number: number

  • n/a

Checklist

  • Update CHANGELOG.md

@Carter12s
Copy link
Collaborator

Ahhh this is a really cool and very interesting change, and something I'd been playing with myself and debating what the right approach was.

I've been trying to figure out how to allow the service function itself to be async, since I'm pushing for an "async first" design style with this crate, but haven't yet figured out how to "do that right" giving evolving interfaces in the rust ecosystem.

Readings if you are interested here:

I think what you've suggested is obviously better than what we currently have, perfect is the enemy of done, and in the future we can add an "advertise_async_service" alternative api if and when Rust stabilizes something.

@Carter12s
Copy link
Collaborator

Some small changes are requested to PR prior to approval:

  1. Please run cargo fmt and commit the resulting lint fixes, our CI currently rejects code not formatted by cargo fmt.

  2. Please add a short description of your change to CHANGLOG.md

Beyond that every looks good if it passes CI (we'll find out after lint is fixed). I'm also happy to make these small edit myself (if you've given me permission)

@entire
Copy link
Contributor Author

entire commented Oct 23, 2023

As per #108 - I went through the codebase to see how an advertise_async_service function might work, and it was a bit more complicated than I initially thought, so I thought this would be a good starting point.

@Carter12s Re: these points, sounds good - will make the changes as requested

@Carter12s Carter12s merged commit c031195 into RosLibRust:master Oct 24, 2023
5 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.

2 participants