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

Tidy namespaces #582

Closed
wants to merge 1 commit into from
Closed

Tidy namespaces #582

wants to merge 1 commit into from

Conversation

mjcarroll
Copy link
Contributor

Housekeeping to clean up namespaces.

Remove using namespace gz and use nested namespace declarations where possible.

@mjcarroll mjcarroll requested a review from jennuine as a code owner October 5, 2023 20:43
@mjcarroll mjcarroll force-pushed the mjcarroll/tidy_namespace branch 2 times, most recently from 187bbf1 to b6011a8 Compare October 5, 2023 21:10
@codecov
Copy link

codecov bot commented Oct 5, 2023

Codecov Report

Merging #582 (02260dd) into gz-gui8 (c01d327) will decrease coverage by 0.01%.
Report is 8 commits behind head on gz-gui8.
The diff coverage is 89.88%.

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

@@             Coverage Diff             @@
##           gz-gui8     #582      +/-   ##
===========================================
- Coverage    68.10%   68.09%   -0.01%     
===========================================
  Files           38       39       +1     
  Lines         5367     5372       +5     
===========================================
+ Hits          3655     3658       +3     
- Misses        1712     1714       +2     
Files Coverage Δ
include/gz/gui/GuiEvents.hh 100.00% <100.00%> (ø)
include/gz/gui/MainWindow.hh 100.00% <100.00%> (ø)
include/gz/gui/Plugin.hh 100.00% <100.00%> (ø)
src/Application.cc 84.13% <100.00%> (+0.03%) ⬆️
src/Conversions.cc 95.18% <100.00%> (ø)
src/Dialog.cc 100.00% <ø> (ø)
src/DragDropModel.cc 100.00% <ø> (ø)
src/GuiEvents.cc 100.00% <ø> (ø)
src/InstallationDirectories.cc 100.00% <100.00%> (ø)
src/MainWindow.cc 95.68% <ø> (ø)
... and 23 more

@mjcarroll mjcarroll force-pushed the mjcarroll/tidy_namespace branch from b6011a8 to 8b9677b Compare October 5, 2023 21:51
Base automatically changed from ports/8_to_main to main October 5, 2023 22:33
@mjcarroll
Copy link
Contributor Author

@osrf-jenkins retest this please

@mjcarroll mjcarroll changed the base branch from main to gz-gui8 October 6, 2023 17:50
@mjcarroll mjcarroll self-assigned this Oct 13, 2023
@@ -37,191 +37,188 @@ namespace tinyxml2
class XMLElement;
}

namespace gz
namespace gz::gui
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we plan to update all the libs within the gazebo ecosystem? If not, I'm slightly against making these changes so that we can keep all the libs in similar style

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Aspirationally, I would like to. Realistically, it takes time. I think that the more condensed looks and feels better, so moving in that direction over time seems appealing.

Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a reason we aren't starting at gz-gui7 or earlier? Will this make forward ports more painful?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think we need at least C++17, so targeting gui7 would be reasonable.

Copy link
Contributor

Choose a reason for hiding this comment

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

let's do that. Also, we should create an issue that tracks the changes for all the other libs with a 'help wanted' label. It could be easy tasks for outside contributors

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just opening another PR seems easier: #590

Signed-off-by: Michael Carroll <[email protected]>
@mjcarroll mjcarroll force-pushed the mjcarroll/tidy_namespace branch from 3e5c68e to 3290544 Compare October 13, 2023 18:26
@mjcarroll mjcarroll closed this Oct 30, 2023
@mjcarroll mjcarroll mentioned this pull request Oct 30, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

2 participants