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.

Sign up for GitHub

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

Jump to bottom

ign -> gz Header Migration : ign-math #413

Merged
merged 4 commits into from
May 2, 2022
Merged

ign -> gz Header Migration : ign-math #413

merged 4 commits into from
May 2, 2022

Conversation

methylDragon
Copy link
Contributor

@methylDragon methylDragon requested a review from scpeters as a code owner April 28, 2022 17:23
@github-actions github-actions bot added the 🌱 garden Ignition Garden label Apr 28, 2022
@methylDragon methylDragon mentioned this pull request Apr 28, 2022
Closed
@chapulina chapulina added the ign to gz Renaming Ignition to Gazebo. label Apr 28, 2022
@methylDragon methylDragon marked this pull request as draft April 28, 2022 23:55
@methylDragon methylDragon force-pushed the header_migration branch 4 times, most recently from 1bc7e9a to 253963c Compare April 29, 2022 01:02
@methylDragon
Copy link
Contributor Author

Extra files have been deleted.

I'm not sure if I edited the Bazel buildfile correctly.

scpeters
scpeters reviewed Apr 29, 2022
View reviewed changes
Copy link
Member

@scpeters scpeters left a comment

Choose a reason for hiding this comment

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

I believe this also has some of the same issues as ign-utils:

  • remove ignition/math/math.hh redirection header
  • add ignition/math/detail/Export.hh redirection header

include/ignition/math/MassMatrix3.ipynb Outdated
@@ -0,0 +1,134 @@
{
Copy link
Member

Choose a reason for hiding this comment

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

this file should be deleted

Copy link
Contributor Author

Choose a reason for hiding this comment

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

253963c !

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Noting: git picked up the math.hh deletion and Export creation as a move for some reason.

@methylDragon
Copy link
Contributor Author

Ubuntu CI is throwing a warning (Python not found), I don't think this is related to this PR.

@codecov
Copy link

codecov bot commented Apr 29, 2022 •
edited
Loading

Codecov Report

Merging #413 (9bf23b9) into main (9622e9e) will not change coverage.
The diff coverage is n/a.

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

@@           Coverage Diff           @@
##             main     #413   +/-   ##
=======================================
  Coverage   99.84%   99.84%           
=======================================
  Files          49       49           
  Lines        4396     4396           
=======================================
  Hits         4389     4389           
  Misses          7        7
Impacted Files Coverage Δ
...include/ignition/math7/ignition/math/detail/Box.hh
...tall/include/ignition/math7/ignition/math/Angle.hh
...nclude/ignition/math7/ignition/math/OrientedBox.hh
...ignition/math7/ignition/math/eigen3/Conversions.hh
...ll/include/ignition/math7/ignition/math/Matrix4.hh
.../include/ignition/math7/ignition/math/Triangle3.hh
...7/ignition/math/AdditivelySeparableScalarField3.hh
...l/include/ignition/math7/ignition/math/Interval.hh
...tall/include/ignition/math7/ignition/math/Plane.hh
...l/include/ignition/math7/ignition/math/Cylinder.hh
... and 82 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9622e9e...f243dd3. Read the comment docs.

@methylDragon
Copy link
Contributor Author

We just need #414 merged for CI to pass, I think.

@methylDragon methylDragon force-pushed the header_migration branch 2 times, most recently from 15016d3 to b6da548 Compare April 30, 2022 01:00
@methylDragon methylDragon marked this pull request as ready for review April 30, 2022 01:09
scpeters
scpeters reviewed Apr 30, 2022
View reviewed changes
include/ignition/math/eigen3.hh Outdated
@@ -0,0 +1,18 @@
/*
Copy link
Member

Choose a reason for hiding this comment

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

I would have expected this file in the eigen/include/ignition/math folder

Copy link
Contributor Author

@methylDragon methylDragon May 2, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Updated! cf5dba7

@methylDragon
Move header files with git mv
85a8dcd 
Signed-off-by: methylDragon <methylDragon@gmail.com>
@methylDragon
Create redirection aliases
cf5dba7 
Signed-off-by: methylDragon <methylDragon@gmail.com>
@methylDragon
Migrate sources in src, test, examples, and include
d9ef5fe 
Signed-off-by: methylDragon <methylDragon@gmail.com>
@methylDragon
Migrate build files
f243dd3 
Signed-off-by: methylDragon <methylDragon@gmail.com>
@methylDragon methylDragon merged commit 9b7714c into main May 2, 2022
@methylDragon methylDragon deleted the header_migration branch May 2, 2022 21:39
@scpeters
Copy link
Member

scpeters commented May 3, 2022

I think we missed the headers from #412 when rebasing

@methylDragon methylDragon mentioned this pull request May 3, 2022
Merged
@scpeters scpeters mentioned this pull request May 9, 2022
Merged
8 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@scpeters scpeters scpeters approved these changes

Assignees
No one assigned
Labels
🌱 garden Ignition Garden ign to gz Renaming Ignition to Gazebo.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@methylDragon @scpeters @chapulina