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

Add async support for subscribing messages #375

Closed
wants to merge 1 commit into from

Conversation

Ryanf55
Copy link

@Ryanf55 Ryanf55 commented Aug 16, 2023

This is an alternative to #356, used for discussion. It doesn't pass because the wait blocks.

* Can now wait without subscribing
* Can now check message contents

Signed-off-by: Ryan Friedman <ryan.friedman+github@avinc.com>
@Ryanf55 Ryanf55 marked this pull request as draft August 16, 2023 21:16
@Ryanf55
Copy link
Author

Ryanf55 commented Aug 16, 2023

@LastStarDust FYI, here is another approach without using callbacks. Note how I also create a publisher in the test python code rather than calling the CI, which is more conducive to an application where you want to set specific fields in the test.

@LastStarDust
Copy link
Contributor

@Ryanf55 Thank you for letting me know. I have some comments about the code, but before I invest any time in this I want to hear the opinion of the maintainers.

@Ryanf55
Copy link
Author

Ryanf55 commented Jan 15, 2024

@Ryanf55 Thank you for letting me know. I have some comments about the code, but before I invest any time in this I want to hear the opinion of the maintainers.

Looks like there are not really any active maintainers who have time to discuss this support. Are you still interested in getting better subscriber testing on humble?

@LastStarDust
Copy link
Contributor

LastStarDust commented Jan 16, 2024

@Ryanf55 for sure I am still interested in getting better subscriber testing on humble.

@project owners
If there are no active maintainers or if they have no time for this feature, would you let us take care of this? In other words, would you give @Ryanf55 and @LastStarDust temporary maintainer role?

@clalancette
Copy link
Contributor

If there are no active maintainers or if they have no time for this feature, would you let us take care of this? In other words, would you give @Ryanf55 and @LastStarDust temporary maintainer role?

There are active maintainers of this repository. In general, this is very core ROS 2 stuff, so I don't think giving temporary maintainer role is suitable here.

That said, at the very least a new feature needs to target rolling first. Once this is targeted at rolling, I'll try to get a maintainer to look at it.

@LastStarDust
Copy link
Contributor

If there are no active maintainers or if they have no time for this feature, would you let us take care of this? In other words, would you give @Ryanf55 and @LastStarDust temporary maintainer role?

There are active maintainers of this repository. In general, this is very core ROS 2 stuff, so I don't think giving temporary maintainer role is suitable here.

That said, at the very least a new feature needs to target rolling first. Once this is targeted at rolling, I'll try to get a maintainer to look at it.

Fair enough. BTW my PR is already targeting rolling.

@Ryanf55 Ryanf55 closed this Feb 20, 2024
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.

3 participants