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

Fixed file path in errors when parsing sdf or urdf from string #537

Merged
merged 8 commits into from
Apr 28, 2021

Conversation

aaronchongth
Copy link
Collaborator

@aaronchongth aaronchongth commented Apr 7, 2021

Signed-off-by: Aaron Chong [email protected]

🦟 Bug fix

Fixes #536

Summary

  • Using fixed strings to highlight string inputs for sdf or urdf
  • Error checks FilePath during construction
  • Added tests

Checklist

  • Signed all commits for DCO
  • Added tests
  • Updated documentation (as needed)
  • Updated migration guide (as needed)
  • Code check passed (In source directory, run sh tools/code_check.sh)
  • All tests passed (See
    test coverage)
  • While waiting for a review on your PR, please help review
    another open pull request
    to support the maintainers

Note to maintainers: Remember to use Squash-Merge

@github-actions github-actions bot added 🏢 edifice Ignition Edifice 🏯 fortress Ignition Fortress labels Apr 7, 2021
@aaronchongth aaronchongth requested a review from azeey April 7, 2021 07:40
Copy link
Collaborator

@EricCousineau-TRI EricCousineau-TRI left a comment

Choose a reason for hiding this comment

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

I've posted some feedback in #536; can you look at it and let me know what you think?

@chapulina chapulina removed the 🏯 fortress Ignition Fortress label Apr 20, 2021
@codecov-commenter
Copy link

codecov-commenter commented Apr 22, 2021

Codecov Report

Merging #537 (b922496) into sdf11 (25004f0) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##            sdf11     #537   +/-   ##
=======================================
  Coverage   87.46%   87.47%           
=======================================
  Files          72       72           
  Lines       10349    10352    +3     
=======================================
+ Hits         9052     9055    +3     
  Misses       1297     1297           
Impacted Files Coverage Δ
include/sdf/Types.hh 100.00% <ø> (ø)
src/Element.cc 97.50% <100.00%> (ø)
src/parser.cc 83.26% <100.00%> (+0.05%) ⬆️

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 25004f0...b922496. Read the comment docs.

… checks FilePath during construction, added tests

Signed-off-by: Aaron Chong <[email protected]>
…ti-line and single line string

Signed-off-by: Aaron Chong <[email protected]>
Signed-off-by: Aaron Chong <[email protected]>
Copy link
Collaborator

@EricCousineau-TRI EricCousineau-TRI left a comment

Choose a reason for hiding this comment

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

LGTM! Just made some comments about GSG violations, but not sure if they're super important w.r.t. current adherence in libsdformat. Thanks!

include/sdf/Filesystem.hh Outdated Show resolved Hide resolved
include/sdf/Filesystem.hh Outdated Show resolved Hide resolved
include/sdf/Filesystem.hh Outdated Show resolved Hide resolved
include/sdf/Filesystem.hh Outdated Show resolved Hide resolved
@aaronchongth
Copy link
Collaborator Author

@osrf-jenkins run test

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.

Looks good. Just a minor comment about where constants are defined.

src/Element.cc Outdated Show resolved Hide resolved
@aaronchongth aaronchongth merged commit 689ee84 into sdf11 Apr 28, 2021
@aaronchongth aaronchongth deleted the aaron/fix-string-parsing-error-bug branch April 28, 2021 23:21
@azeey
Copy link
Collaborator

azeey commented Apr 28, 2021

oops, @aaronchongth please use squash merge next time.

@aaronchongth
Copy link
Collaborator Author

@azeey I apologize, I thought I selected squash merge for the auto-merging, I'll do it manually in the future just in case

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.

String parsing results in FilePath as data-string when error occurs
5 participants