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

refactor(autoware_tracking_object_merger): move headers to include/autoware and rename package #7809

Conversation

esteve
Copy link
Contributor

@esteve esteve commented Jul 3, 2024

Description

move headers to include/autoware and rename package

Related links

Parent Issue:

How was this PR tested?

Notes for reviewers

None.

Interface changes

None.

Effects on system behavior

None.

@esteve esteve enabled auto-merge (squash) July 3, 2024 11:41
@github-actions github-actions bot added type:documentation Creating or refining documentation. (auto-assigned) component:perception Advanced sensor data processing and environment understanding. (auto-assigned) type:ci Continuous Integration (CI) processes and testing. (auto-assigned) component:launch Launch files, scripts and initialization tools. (auto-assigned) labels Jul 3, 2024
Copy link

github-actions bot commented Jul 3, 2024

Thank you for contributing to the Autoware project!

🚧 If your pull request is in progress, switch it to draft mode.

Please ensure:

@esteve esteve added the tag:run-build-and-test-differential Mark to enable build-and-test-differential workflow. (used-by-ci) label Jul 3, 2024
Copy link
Contributor

@technolojin technolojin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. package renaming
    I am thinking to do it after all the other packages are ready.

  2. moving headers
    the association class will be replaced by shared library. mentioned in refactor(autoware_object_merger): move headers to src and rename package #7804 (review)

@esteve
Copy link
Contributor Author

esteve commented Jul 4, 2024

  1. package renaming
    I am thinking to do it after all the other packages are ready.

Do you have an estimate for when this will be done? Why not do it now for each package? We've done it for other subsystems (planning, localization, common, etc.)

  1. moving headers
    the association class will be replaced by shared library. mentioned in refactor(autoware_object_merger): move headers to src and rename package #7804 (review)

It's best to do it now and then if you want to create a third package that provides a shared library, then you can do it, but there's no estimate when the shared library will be available and instead this PR already contains the changes now. You can create the shared library after the changes in this PR (and the other PRs) are merged.

Copy link

codecov bot commented Jul 4, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 28.80%. Comparing base (6e74c19) to head (a2f5d39).

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #7809      +/-   ##
==========================================
- Coverage   28.94%   28.80%   -0.14%     
==========================================
  Files        1606     1615       +9     
  Lines      117652   118195     +543     
  Branches    50632    50746     +114     
==========================================
  Hits        34050    34050              
- Misses      74481    75024     +543     
  Partials     9121     9121              
Flag Coverage Δ *Carryforward flag
differential 0.00% <ø> (?)
total 28.94% <ø> (ø) Carriedforward from 6e74c19

*This pull request uses carry forward flags. Click here to find out more.

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

@technolojin
Copy link
Contributor

@esteve

  1. please see Prefix packages with autoware_ prefix and keep code in autoware namespace autoware#4569 (comment)
  2. Relocating the headers is optional. The other components (planning, localization) it was not done. Why you want to do it in the same time? It was not mentioned in your ISSUES.

@mitsudome-r
Copy link
Member

@esteve
Thanks for creating the PR.
It seems like @technolojin would like to split this PR into two. Could you drop the changes with the change in header location and just rename the package in this PR?

@esteve
Copy link
Contributor Author

esteve commented Jul 11, 2024

@mitsudome-r sure, no problem. However, the headers need to be moved anyway, either to a private location in src (what I did in this PR), or to a public one include/autoware/tracking_object_merger, so I used the opportunity to move them to a private location. I'll move the headers to the latter to keep them public.

@esteve esteve marked this pull request as draft July 16, 2024 12:52
auto-merge was automatically disabled July 16, 2024 12:52

Pull request was converted to draft

@esteve esteve force-pushed the fix-include-dir-autoware_tracking_object_merger branch from dcfebc0 to 9359e4f Compare July 16, 2024 12:54
@esteve esteve marked this pull request as ready for review July 16, 2024 13:02
@technolojin technolojin self-assigned this Jul 17, 2024
@esteve esteve changed the title refactor(autoware_tracking_object_merger): move headers to src and rename package refactor(autoware_tracking_object_merger): move headers to include/autoware and rename package Jul 17, 2024
…toware and rename package

Signed-off-by: Esteve Fernandez <esteve.fernandez@tier4.jp>
@esteve esteve force-pushed the fix-include-dir-autoware_tracking_object_merger branch from 471d5e7 to a2f5d39 Compare July 17, 2024 12:40
@esteve esteve enabled auto-merge (squash) July 17, 2024 12:40
Copy link
Contributor

@technolojin technolojin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you fix the launchers?

$(find-pkg-share tracking_object_merger) -> $(find-pkg-share autoware_tracking_object_merger)

Signed-off-by: Taekjin LEE <taekjin.lee@tier4.jp>
Copy link
Contributor

@technolojin technolojin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I fixed the launchers.

@esteve esteve merged commit 89a130a into autowarefoundation:main Jul 19, 2024
22 of 23 checks passed
@esteve esteve deleted the fix-include-dir-autoware_tracking_object_merger branch July 19, 2024 01:16
yhisaki pushed a commit to yhisaki/autoware.universe that referenced this pull request Jul 19, 2024
…toware and rename package (autowarefoundation#7809)

Signed-off-by: Esteve Fernandez <esteve.fernandez@tier4.jp>
Signed-off-by: Y.Hisaki <yhisaki31@gmail.com>
Ariiees pushed a commit to Ariiees/autoware.universe that referenced this pull request Jul 22, 2024
…toware and rename package (autowarefoundation#7809)

Signed-off-by: Esteve Fernandez <esteve.fernandez@tier4.jp>
chihungtzeng pushed a commit to chihungtzeng/autoware.universe that referenced this pull request Jul 23, 2024
…toware and rename package (autowarefoundation#7809)

Signed-off-by: Esteve Fernandez <esteve.fernandez@tier4.jp>
Signed-off-by: chtseng <chtseng@itri.org.tw>
esteve added a commit to esteve/autoware.universe that referenced this pull request Aug 13, 2024
…toware and rename package (autowarefoundation#7809)

Signed-off-by: Esteve Fernandez <esteve.fernandez@tier4.jp>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component:launch Launch files, scripts and initialization tools. (auto-assigned) component:perception Advanced sensor data processing and environment understanding. (auto-assigned) tag:run-build-and-test-differential Mark to enable build-and-test-differential workflow. (used-by-ci) type:ci Continuous Integration (CI) processes and testing. (auto-assigned) type:documentation Creating or refining documentation. (auto-assigned)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants