-
Notifications
You must be signed in to change notification settings - Fork 676
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
feat: add published_time publisher debug to packages #6490
feat: add published_time publisher debug to packages #6490
Conversation
9958bd0
to
33f59b0
Compare
This failed but probably a flaky test. Opened an issue for it: |
perception/map_based_prediction/include/map_based_prediction/map_based_prediction_node.hpp
Outdated
Show resolved
Hide resolved
perception/map_based_prediction/include/map_based_prediction/map_based_prediction_node.hpp
Outdated
Show resolved
Hide resolved
perception/map_based_prediction/src/map_based_prediction_node.cpp
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for all the work. I've made my review, most of them are for specific parts.
But following comments should be removed from everywhere:
// Publish published time only if there are subscribers more than 1
// PublishedTime Publisher
https://stackoverflow.blog/2021/12/23/best-practices-for-writing-code-comments/
Otherwise you are violating the Rule #1
of writing code comments.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #6490 +/- ##
==========================================
- Coverage 14.79% 14.78% -0.01%
==========================================
Files 1920 1920
Lines 132385 132447 +62
Branches 39345 39378 +33
==========================================
Hits 19585 19585
- Misses 90956 90991 +35
- Partials 21844 21871 +27
*This pull request uses carry forward flags. Click here to find out more. ☔ View full report in Codecov by Sentry. |
Signed-off-by: Berkay Karaman <[email protected]>
Signed-off-by: M. Fatih Cırıt <[email protected]>
49e7c45
to
be994ed
Compare
Signed-off-by: Berkay Karaman <[email protected]>
081eaaa
to
752bb0b
Compare
@brkay54 Could you go through all the changed packages to make sure they have |
Signed-off-by: Berkay Karaman <[email protected]>
@brkay54 thanks, Right now only way for this PR to break existing behavior or even crash the modified nodes is: If Could you make sure every modified node initializes this I don't know if there is a way to check this in a more automated way. |
I've checked all modified Waiting for the re-run test that had failed from an unrelated flaky test. |
…efoundation#6490)" This reverts commit 7f36c52.
…ion#6490) Signed-off-by: Berkay Karaman <[email protected]> Signed-off-by: kaigohirao <[email protected]>
…ion#6490) Signed-off-by: Berkay Karaman <[email protected]>
Description
Part of:
Depends on:
Related links
Tests performed
Tested on PSim and e2e_simulator, worked well.
Notes for reviewers
Interface changes
Effects on system behavior
Pre-review checklist for the PR author
The PR author must check the checkboxes below when creating the PR.
In-review checklist for the PR reviewers
The PR reviewers must check the checkboxes below before approval.
Post-review checklist for the PR author
The PR author must check the checkboxes below before merging.
After all checkboxes are checked, anyone who has write access can merge the PR.