-
Notifications
You must be signed in to change notification settings - Fork 37
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: migrate autoware_localization_util from universe to core #150
base: main
Are you sure you want to change the base?
feat: migrate autoware_localization_util from universe to core #150
Conversation
Signed-off-by: liuXinGangChina <[email protected]>
Thank you for contributing to the Autoware project! 🚧 If your pull request is in progress, switch it to draft mode. Please ensure:
|
...ization/autoware_localization_util/include/autoware/localization_util/diagnostics_module.hpp
Outdated
Show resolved
Hide resolved
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #150 +/- ##
==========================================
- Coverage 78.75% 70.74% -8.02%
==========================================
Files 11 18 +7
Lines 193 564 +371
Branches 73 201 +128
==========================================
+ Hits 152 399 +247
- Misses 11 107 +96
- Partials 30 58 +28
*This pull request uses carry forward flags. Click here to find out more. ☔ View full report in Codecov by Sentry. |
…dule from localization_util : v0.1 Signed-off-by: liuXinGangChina <[email protected]>
f496f24
to
c12f241
Compare
Signed-off-by: liuXinGangChina <[email protected]>
Signed-off-by: liuXinGangChina <[email protected]>
058a8e9
to
0ea300c
Compare
Signed-off-by: liuXinGangChina <[email protected]>
Signed-off-by: liuXinGangChina <[email protected]>
So far, there is only one required item of ci failed (cpp-check-differicial),below is the report it looks no node use these functions, but i believe they will be used in the future because there are more nodes to be migrate into core In conclusion, I think we can merge this pr for now。 What’s your idea,shintaro san @SakodaShintaro ? Have a nice day! |
Thank you for the correction. Yes, those functions are used in autoware.universe. There seem to be some errors when implementing functions that are only used in autoware.universe. Is there a policy on for how to deal with these? I feel that it is questionable whether such functions and packages should be migrated to autoware.core. |
@SakodaShintaro Currently, the This is because the differential checks cannot avoid false-positives of It seems that the cppcheck configuration in autoware.core do not ignore |
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.
Since it’s a porting task, I’m keeping the review to a minimum.
However I was concerned about the lack of consistency, as the existing package names have a _utils
suffix, whereas this one uses _util
.
Another pull request in autoware.universe to remove Could you create the PR and link to it in the description? |
…ion to 0.0.0: v0.6 Signed-off-by: liuXinGangChina <[email protected]>
Shintaro san sure ,i will create a pr under universe repo have a nice day! 心刚 |
… node compatibility: v0.7 Signed-off-by: liuXinGangChina <[email protected]>
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.
Thank you for the revision. LGTM
Shintaro san the pr to remove original package under universe repo is autowarefoundation/autoware.universe#9888 Best regards 心刚 |
Description
Port autoware_localization_util from Autoware.Universe to Core.
Related links
Parent Issue:
How was this PR tested?
Notes for reviewers
None.
Interface changes
None.
Effects on system behavior
None.