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 : Remove redundant namespace references #414

Merged
merged 1 commit into from
Aug 10, 2022
Merged

ign -> gz : Remove redundant namespace references #414

merged 1 commit into from
Aug 10, 2022

Conversation

methylDragon
Copy link
Contributor

@methylDragon methylDragon requested a review from mjcarroll as a code owner August 9, 2022 03:50
@methylDragon methylDragon changed the title Citadel -> Fortress: Remove redundant namespace references ign -> gz : Remove redundant namespace references Aug 9, 2022
@methylDragon methylDragon mentioned this pull request Aug 9, 2022
Open
@github-actions github-actions bot added the 🏰 citadel Ignition Citadel label Aug 9, 2022
@methylDragon methylDragon force-pushed the namespace_cleanup branch 2 times, most recently from 54de82a to e103a2b Compare August 9, 2022 04:39
chapulina
chapulina suggested changes Aug 9, 2022
View reviewed changes
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.

Compilation is failing, we need to make sure the namespace is used where we remove it:

create_and_switch_to_temp_dir(std::__cxx11::string&)':
  /github/workspace/src/Filesystem_TEST.cc:38:8: error: 'env' was not declared in this scope
     if (!env("TMPDIR", tmppath))

src/PluginUtils_TEST.cc Outdated Show resolved Hide resolved
@methylDragon methylDragon force-pushed the namespace_cleanup branch 2 times, most recently from ac455da to 37070a9 Compare August 9, 2022 19:26
chapulina
chapulina suggested changes Aug 9, 2022
View reviewed changes
src/Console_TEST.cc Outdated
@@ -40,12 +40,12 @@ class Console_TEST : public ::testing::Test {
/// \brief Clear out all the directories we produced during this test.
public: virtual ~Console_TEST()
{
EXPECT_TRUE(ignition::common::unsetenv(IGN_HOMEDIR));
EXPECT_TRUE(unsetenv(IGN_HOMEDIR));
Copy link
Contributor

Choose a reason for hiding this comment

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

This is one of the places where removing the namespace is actually changing what function is called. I believe the tests are failing because this is calling this instead of our cross-platform function.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm.. Let me add the following excludes then:

  • env
  • unsetenv
  • joinPaths
  • removeAll
  • uuid
  • isDirectory

This will result in just ignition:: being removed, where applicable

@chapulina chapulina mentioned this pull request Aug 9, 2022
Merged
@methylDragon
Remove redundant namespace references
6844807 
Signed-off-by: methylDragon <methylDragon@gmail.com>
@codecov
Copy link

codecov bot commented Aug 9, 2022 •
edited
Loading

Codecov Report

Merging #414 (6844807) into ign-common3 (0f7ec78) will not change coverage.
The diff coverage is 86.78%.

@@             Coverage Diff              @@
##           ign-common3     #414   +/-   ##
============================================
  Coverage        78.52%   78.52%           
============================================
  Files               72       72           
  Lines             9244     9244           
============================================
  Hits              7259     7259           
  Misses            1985     1985
Impacted Files Coverage Δ
graphics/src/BVHLoader.cc 0.00% <0.00%> (ø)
graphics/src/GTSMeshUtils.cc 95.41% <ø> (ø)
graphics/src/ImageHeightmap.cc 88.46% <ø> (ø)
graphics/src/MeshCSG.cc 0.00% <0.00%> (ø)
graphics/src/STLLoader.cc 3.31% <0.00%> (ø)
av/src/HWEncoder.cc 20.00% <33.33%> (ø)
graphics/src/Animation.cc 91.90% <50.00%> (ø)
graphics/src/Image.cc 68.75% <50.00%> (ø)
graphics/src/SVGLoader.cc 86.83% <73.21%> (ø)
src/Console.cc 86.71% <75.00%> (ø)
... and 15 more

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

@methylDragon
Copy link
Contributor Author

Tests now pass, but with a deprecation warning on homebrew (that I think is unrelated?)

@methylDragon methylDragon requested a review from chapulina August 10, 2022 00:39
@chapulina
Copy link
Contributor

deprecation warning on homebrew (that I think is unrelated?)

It looks like @mjcarroll is on it

@chapulina chapulina added the ign to gz Renaming Ignition to Gazebo. label Aug 10, 2022
@chapulina chapulina merged commit c05a3bb into ign-common3 Aug 10, 2022
@chapulina chapulina deleted the namespace_cleanup branch August 10, 2022 02:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@chapulina chapulina chapulina approved these changes

@mjcarroll mjcarroll Awaiting requested review from mjcarroll

Assignees
No one assigned
Labels
🏰 citadel Ignition Citadel ign to gz Renaming Ignition to Gazebo.
Projects
Core development
Archived in project
Milestone
No milestone
Development

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