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

Make dartsim/World.hh header private #551

Merged
merged 3 commits into from
Sep 26, 2023

Conversation

j-rivero
Copy link
Contributor

Move dartsim/World.hh header out from public headers to be in src/ as a private header as suggested long ago in #274 (comment)

Checklist

  • Signed all commits for DCO
  • Added tests
  • Added example and/or tutorial
  • Updated documentation (as needed)
  • Updated migration guide (as needed)
  • Consider updating Python bindings (if the library has them)
  • codecheck passed (See contributing)
  • All tests passed (See test coverage)
  • While waiting for a review on your PR, please help review another open pull request to support the maintainers

Note to maintainers: Remember to use Squash-Merge and edit the commit message to match the pull request summary while retaining Signed-off-by messages.

Signed-off-by: Jose Luis Rivero <[email protected]>
@github-actions github-actions bot added the 🎵 harmonic Gazebo Harmonic label Sep 25, 2023
@codecov
Copy link

codecov bot commented Sep 25, 2023

Codecov Report

Merging #551 (8961281) into gz-physics7 (9165154) will not change coverage.
Report is 1 commits behind head on gz-physics7.
The diff coverage is n/a.

❗ Current head 8961281 differs from pull request most recent head ab38813. Consider uploading reports for the commit ab38813 to get more accurate results

@@             Coverage Diff              @@
##           gz-physics7     #551   +/-   ##
============================================
  Coverage        76.89%   76.89%           
============================================
  Files              140      140           
  Lines             7683     7683           
============================================
  Hits              5908     5908           
  Misses            1775     1775           
Files Coverage Δ
dartsim/src/World.hh 100.00% <ø> (ø)

@azeey
Copy link
Contributor

azeey commented Sep 26, 2023

After this PR, would the libgz-physics7-dartsim-dev package become obsolete?

@scpeters
Copy link
Member

After this PR, would the libgz-physics7-dartsim-dev package become obsolete?

that still holds the cmake config files, so I think it's needed?

@j-rivero
Copy link
Contributor Author

After this PR, would the libgz-physics7-dartsim-dev package become obsolete?

that still holds the cmake config files, so I think it's needed?

* https://github.com/gazebo-release/gz-physics7-release/blob/main/ubuntu/debian/libgz-physics-dartsim-dev.install 

that still holds the cmake config files, so I think it's needed?

This is a good point. I think that the main question is: is there a valid use for developers that would generate code from the dartsim plugin that has no public headers? Because we are generating the buildsystem helpers (CMake + pkgconfig) to facilitate that use case. If there is no use case, we either not packaging the helpers and remove the -dev package or change our buildsystem not to generate the helpers.

@j-rivero j-rivero merged commit 578b4ed into gz-physics7 Sep 26, 2023
4 checks passed
@j-rivero j-rivero deleted the jrivero/dartsim_header_move branch September 26, 2023 21:39
@j-rivero
Copy link
Contributor Author

j-rivero commented Sep 26, 2023

Let's move the conversation to #541

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🎵 harmonic Gazebo Harmonic
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants