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

ign -> gz : Remove redundant namespace references #1105

Closed
wants to merge 1 commit into from

Conversation

methylDragon
Copy link
Contributor

@codecov
Copy link

codecov bot commented Aug 9, 2022

Codecov Report

Merging #1105 (7e3ccfe) into sdf9 (3e76d92) will not change coverage.
The diff coverage is 86.01%.

@@           Coverage Diff           @@
##             sdf9    #1105   +/-   ##
=======================================
  Coverage   87.59%   87.59%           
=======================================
  Files          64       64           
  Lines       10061    10061           
=======================================
  Hits         8813     8813           
  Misses       1248     1248           
Impacted Files Coverage Δ
src/Sensor.cc 77.70% <30.00%> (ø)
src/Geometry.cc 87.59% <42.85%> (ø)
src/Model.cc 83.70% <50.00%> (ø)
src/Camera.cc 81.16% <70.00%> (ø)
src/World.cc 93.45% <77.77%> (ø)
src/Pbr.cc 98.49% <83.33%> (ø)
src/Console.cc 87.71% <85.71%> (ø)
src/Actor.cc 83.33% <100.00%> (ø)
src/AirPressure.cc 100.00% <100.00%> (ø)
src/Altimeter.cc 100.00% <100.00%> (ø)
... and 32 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@github-actions github-actions bot added Gazebo 1️1️ Dependency of Gazebo classic version 11 🏰 citadel Ignition Citadel labels Aug 9, 2022
Signed-off-by: methylDragon <methylDragon@gmail.com>
@chapulina chapulina added the ign to gz Renaming Ignition to Gazebo. label Aug 9, 2022
@scpeters
Copy link
Member

scpeters commented Aug 9, 2022

I think the namespace reduction plan in gazebo-tooling/release-tools#784 is useful for eliminating ignition:: namespaces, but for this library it is just removing sdf:: in lots of places, which looks like noise to me. I would rather just decline this.

Copy link
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.

This PR isn't strictly helpful for the ignition migration because this repository doesn't release any code under that namespace, so we shouldn't be using namespace ignition here.

But I think removing those redundant namespaces doesn't hurt, makes some liner shorter, and the code a bit cleaner. It may just introduce some confusion for functions that aren't clearly from this library, like sdf::split

@chapulina
Copy link
Contributor

I just saw @scpeters 's comment, I'm fine declining this PR if that's preferred.

@methylDragon
Copy link
Contributor Author

I have no preferences, but if needed, I can exclude stuff like split from the cleanup, it's not too difficult for me to do (like a 5-10 min job)

@chapulina
Copy link
Contributor

@methylDragon , I'll just close this one for expediency. We don't need to put more time into it. Let's focus on ignition:: and ignition::gazebo.

@chapulina chapulina closed this Aug 10, 2022
@chapulina chapulina deleted the namespace_cleanup branch August 10, 2022 15:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏰 citadel Ignition Citadel Gazebo 1️1️ Dependency of Gazebo classic version 11 ign to gz Renaming Ignition to Gazebo.
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants