Skip to content

ign -> gz Namespace Migration : gz-math#430

Merged
chapulina merged 24 commits intomainfrom
namespace_migration
May 28, 2022
Merged

ign -> gz Namespace Migration : gz-math#430
chapulina merged 24 commits intomainfrom
namespace_migration

Conversation

@methylDragon
Copy link
Copy Markdown
Contributor

Signed-off-by: methylDragon <methylDragon@gmail.com>
Signed-off-by: methylDragon <methylDragon@gmail.com>
Signed-off-by: methylDragon <methylDragon@gmail.com>
Signed-off-by: methylDragon <methylDragon@gmail.com>
Signed-off-by: methylDragon <methylDragon@gmail.com>
Signed-off-by: methylDragon <methylDragon@gmail.com>
@methylDragon methylDragon force-pushed the namespace_migration branch from 3b493fe to 801dc4b Compare May 18, 2022 05:19
Signed-off-by: methylDragon <methylDragon@gmail.com>
@methylDragon methylDragon force-pushed the namespace_migration branch from 6b15d1f to b421b81 Compare May 18, 2022 06:26
@chapulina chapulina added ign to gz Renaming Ignition to Gazebo. needs upstream release Blocked by a release of an upstream library labels May 18, 2022
Signed-off-by: methylDragon <methylDragon@gmail.com>
Signed-off-by: methylDragon <methylDragon@gmail.com>
Signed-off-by: methylDragon <methylDragon@gmail.com>
Signed-off-by: methylDragon <methylDragon@gmail.com>
Signed-off-by: methylDragon <methylDragon@gmail.com>
Copy link
Copy Markdown
Contributor

@chapulina chapulina left a comment

Choose a reason for hiding this comment

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

LGTM with 🟢 CI, just minor comments

@@ -1,4 +1,4 @@
%module "ignition::math"
%module "gz::math"
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I don't know if this has packaging consequences, we may need to update the -release repo after this is merged

Signed-off-by: methylDragon <methylDragon@gmail.com>
Signed-off-by: methylDragon <methylDragon@gmail.com>
Signed-off-by: methylDragon <methylDragon@gmail.com>
Signed-off-by: methylDragon <methylDragon@gmail.com>
Signed-off-by: methylDragon <methylDragon@gmail.com>
Signed-off-by: methylDragon <methylDragon@gmail.com>
///
/// This class will replace the
/// [MaterialDensity class](https://github.com/ignitionrobotics/ign-common/blob/ign-common1/include/gz/common/MaterialDensity.hh)
/// [MaterialDensity class](https://github.com/gazebosim/gz-common/blob/ign-common1/include/gz/common/MaterialDensity.hh)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I just noticed this link was previously broken because the include/gz folder doesn't exist on the ign-common1 branch

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Should I point it to main or point gz back to ignition?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Signed-off-by: methylDragon <methylDragon@gmail.com>
Signed-off-by: methylDragon <methylDragon@gmail.com>
Signed-off-by: methylDragon <methylDragon@gmail.com>
Signed-off-by: methylDragon <methylDragon@gmail.com>
Signed-off-by: methylDragon <methylDragon@gmail.com>
Copy link
Copy Markdown
Contributor

@chapulina chapulina left a comment

Choose a reason for hiding this comment

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

Nice! Just pending 🟢 CI

Signed-off-by: methylDragon <methylDragon@gmail.com>
Copy link
Copy Markdown
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 think we shoudn't change the namespace references in the parts of our migration guide that refers to old versions.

I'm also not sure we should change the changelog to refer to old versions (like Gazebo math 2) when we aren't changing that code at all. Are we allowed to historically refer to old versions as ignition, or does it all have to change to gazebo?

@scpeters
Copy link
Copy Markdown
Member

I think we shoudn't change the namespace references in the parts of our migration guide that refers to old versions.

I'm also not sure we should change the changelog to refer to old versions (like Gazebo math 2) when we aren't changing that code at all. Are we allowed to historically refer to old versions as ignition, or does it all have to change to gazebo?

having discussed this further offline, I think the changes are fine as is

@codecov
Copy link
Copy Markdown

codecov bot commented May 28, 2022

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 99.84%. Comparing base (84e650a) to head (b9dff19).
⚠️ Report is 301 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #430   +/-   ##
=======================================
  Coverage   99.84%   99.84%           
=======================================
  Files          49       49           
  Lines        4396     4396           
=======================================
  Hits         4389     4389           
  Misses          7        7           

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

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@chapulina chapulina merged commit d6284a5 into main May 28, 2022
@chapulina chapulina deleted the namespace_migration branch May 28, 2022 16:32
@chapulina chapulina removed the needs upstream release Blocked by a release of an upstream library label May 29, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

🌱 garden Ignition Garden ign to gz Renaming Ignition to Gazebo.

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

4 participants