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

Add split/join for separating the link name from it's fully qualified name #457

Merged
merged 5 commits into from
Jan 27, 2021

Conversation

brawner
Copy link
Collaborator

@brawner brawner commented Jan 9, 2021

This PR adds small functionality for splitting and joining a name based on the '::' separator. It's necessary for gazebosim/gz-sim#542

@brawner brawner requested a review from azeey January 9, 2021 00:17
@brawner brawner added the 🏢 edifice Ignition Edifice label Jan 11, 2021
@brawner brawner self-assigned this Jan 11, 2021
@brawner brawner force-pushed the brawner/split_join_name branch from d07a06b to b62f7af Compare January 26, 2021 19:00
@brawner brawner changed the title WIP: Add split/join for separating the link name from it's fully qualified name Add split/join for separating the link name from it's fully qualified name Jan 26, 2021
@brawner brawner marked this pull request as ready for review January 26, 2021 19:02
@brawner brawner force-pushed the brawner/split_join_name branch from cc42b34 to dc2fb5b Compare January 26, 2021 19:37
src/Types.cc Outdated
return false;

const size_t findStartPosition = _s.size() - kSdfScopeDelimiter.size();
return _s.substr(findStartPosition) == kSdfScopeDelimiter;
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This should actually just use std::string::compare(). Same in line 123

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done

Copy link
Collaborator

@azeey azeey left a comment

Choose a reason for hiding this comment

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

LGTM!

@@ -228,6 +231,14 @@ namespace sdf
/// \param[in] _in String to convert to lowercase
/// \return Lowercase equilvalent of _in.
std::string SDFORMAT_VISIBLE lowercase(const std::string &_in);

SDFORMAT_VISIBLE
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you add docs here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Added

src/Types.cc Outdated
return false;

const size_t findStartPosition = _s.size() - kSdfScopeDelimiter.size();
return _s.substr(findStartPosition) == kSdfScopeDelimiter;
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: Probably more efficient to do _s.find(kSdfScopeDelimiter, findStartPosition) == findStartPosition as it avoids constructing a new string.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Totally, switched to std::string::compare

@codecov-io
Copy link

codecov-io commented Jan 26, 2021

Codecov Report

Merging #457 (023f9a7) into master (54a69c5) will increase coverage by 0.03%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #457      +/-   ##
==========================================
+ Coverage   88.10%   88.14%   +0.03%     
==========================================
  Files          66       66              
  Lines        9940     9969      +29     
==========================================
+ Hits         8758     8787      +29     
  Misses       1182     1182              
Impacted Files Coverage Δ
include/sdf/Types.hh 100.00% <ø> (ø)
src/Types.cc 100.00% <100.00%> (ø)

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 54a69c5...023f9a7. Read the comment docs.

azeey and others added 4 commits January 26, 2021 13:41
Signed-off-by: Addisu Z. Taddese <addisu@openrobotics.org>
Signed-off-by: Stephen Brawner <brawner@gmail.com>
Signed-off-by: Stephen Brawner <brawner@gmail.com>
Signed-off-by: Stephen Brawner <brawner@gmail.com>
@brawner brawner force-pushed the brawner/split_join_name branch from 15dd27b to 023f9a7 Compare January 26, 2021 21:41
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.

looks great! just one nit that I'll fix right now

src/Types.cc Outdated
return _s.compare(0, kSdfScopeDelimiter.size(), kSdfScopeDelimiter) == 0;
}

// Join an scope name prefix with a local name using the scope delimeter
Copy link
Member

Choose a reason for hiding this comment

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

nit: an -> a

Copy link
Member

Choose a reason for hiding this comment

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

Signed-off-by: Steve Peters <scpeters@openrobotics.org>
@brawner
Copy link
Collaborator Author

brawner commented Jan 27, 2021

Thanks Steve for fixing the nit!

@brawner brawner merged commit b9fc657 into master Jan 27, 2021
@brawner brawner deleted the brawner/split_join_name branch January 27, 2021 22:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏢 edifice Ignition Edifice
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants