Encode XML path and line number in Element and add XML path setter Error. #548

aaronchongth · 2021-04-27T07:03:29Z

🎉 New feature

Closes #226 and #510

Summary

This encodes the XML path and line number information into each Element during parsing, and further adds the XML path information into the errors that occur, specifically when readXml is called.

Changes to sdf::Error:

Added XmlPath setter and getter to the API
Operator << now handles printouts depending on availability of XmlPath, FilePath and LineNumber
- Error Code #: [XML_PATH]: Msg: ...
- Error Code #: [XML_PATH:FILE_PATH]: Msg: ...
- Error Code #: [XML_PATH:FILE_PATH:L#]: Msg: ...
Added tests

Changes to sdf::Element:

Added API for XmlPath and LineNumber
Added unit tests
Added integration tests, see test/integration/element_tracing.cc

Changes to Utils:

Added XML path support to sdfwarn and sdfdbg
Added tests

Changes to parser:

Keeps track XML path as it parses the SDF
Attributes the XML path to each corresponding SDF Element

Test it

source ws/install/setup.bash
./ws/build/sdformat11/src/UNIT_Error_TEST
./ws/build/sdformat11/src/UNIT_Element_TEST
./ws/build/sdformat11/src/UNIT_Utils_TEST
./ws/build/sdformat11/src/UNIT_parser_TEST
./ws/build/sdformat11/test/integration/INTEGRATION_element_tracing

Testing with ign sdf -k

source ws/install/setup.bash
cd ws/src/sdformat/test/sdf
ign sdf -k bad_syntax_double.sdf

We would expect the top of the error message stack to be,

Error Code 9: [/sdf/world[@name="default"]/model[@name="robot1"]/link[@name="link"]/velocity_decay/linear:/ws/src/sdformat/test/sdf/bad_syntax_double.sdf:L7]: Msg: Error reading element <linear>

Checklist

Signed all commits for DCO
Added tests
Added example and/or tutorial
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

codecov-commenter · 2021-04-27T07:30:09Z

Codecov Report

Merging #548 (2064fa1) into sdf11 (25004f0) will decrease coverage by 0.02%.
The diff coverage is 84.04%.

❗ Current head 2064fa1 differs from pull request most recent head acb8dab. Consider uploading reports for the commit acb8dab to get more accurate results

@@            Coverage Diff             @@
##            sdf11     #548      +/-   ##
==========================================
- Coverage   87.46%   87.43%   -0.03%     
==========================================
  Files          72       72              
  Lines       10349    10421      +72     
==========================================
+ Hits         9052     9112      +60     
- Misses       1297     1309      +12

Impacted Files	Coverage Δ
include/sdf/Element.hh	`100.00% <ø> (ø)`
include/sdf/Error.hh	`60.86% <46.15%> (-14.14%)`	⬇️
src/Utils.cc	`85.93% <75.00%> (-3.35%)`	⬇️
src/parser.cc	`83.61% <96.77%> (+0.40%)`	⬆️
src/Element.cc	`97.54% <100.00%> (+0.04%)`	⬆️
src/Error.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 25004f0...acb8dab. Read the comment docs.

aaronchongth · 2021-04-28T02:00:37Z

@osrf-jenkins run test

azeey

Nice work on constructing the xml path during parsing. I have a couple of minor comments as well a comment about the ABI test failure that we should address. I did notice that in the description of this PR, you said this closes #226, but that issue is about associating line numbers with sdf::Element. Are you planning to add that to this PR or should that come later in another PR?

include/sdf/Element.hh

include/sdf/Error.hh

src/Utils.cc

aaronchongth · 2021-04-29T01:36:39Z

🤦‍♂️ I was working towards #538, and only read #226 bottom few comments. This should be a trivial addition, and can be added in this PR.

src/Element_TEST.cc

EricCousineau-TRI · 2021-05-06T18:51:30Z

FYI @SeanCurtis-TRI

EricCousineau-TRI · 2021-05-06T18:52:54Z

@aaronchongth Can you update PR title to explicitly mention line numbers?

azeey · 2021-05-10T15:06:18Z

Per VC, @aaron is updating the parser to set line numbers on sdf::Element objects and adding integration tests.

jennuine

LGTM! A few minor nit suggestions

include/sdf/Element.hh

include/sdf/Error.hh

src/parser.cc

EricCousineau-TRI · 2021-05-14T09:14:20Z

Can you update PR review to say: Closes #226; Closes #510

I'm not sure if GitHub recognizes / parses and in a functional sense:
https://docs.github.com/en/github/managing-your-work-on-github/linking-a-pull-request-to-an-issue#linking-a-pull-request-to-an-issue-using-a-keyword

EricCousineau-TRI

Awesome! (pending merge/de-conflict and remaining discussion items)

jennuine · 2021-05-14T15:53:46Z

Can you update PR review to say: Closes #226; Closes #510

I'm not sure if GitHub recognizes / parses and in a functional sense:
https://docs.github.com/en/github/managing-your-work-on-github/linking-a-pull-request-to-an-issue#linking-a-pull-request-to-an-issue-using-a-keyword

@EricCousineau-TRI I've linked the issue so it should close both regardless

EricCousineau-TRI · 2021-05-14T16:24:31Z

Ah, sweet - thanks for clarifying / correcting me!

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

aaronchongth · 2021-05-14T20:08:11Z

Apologies for the delay! I had some trouble getting my tests to pass for nested models and just managed to solve it, 1d46a84. Thanks for the reviews so far!

test/integration/element_tracing.cc

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

aaronchongth · 2021-05-15T03:03:33Z

@osrf-jenkins run test

…/model.sdf regardless Signed-off-by: Aaron Chong <[email protected]>

jennuine

Looks good! The only thing that needs changed is the input for sdf::testing::TestFile. The other comments are just a suggestion and minor question.

test/integration/element_tracing.cc

src/Element_TEST.cc

src/parser.cc

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

azeey

Great work!!

…parated arguments in sdf::testing::TestFile Signed-off-by: Aaron Chong <[email protected]>

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

aaronchongth · 2021-05-18T18:08:23Z

@osrf-jenkins run test

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

jennuine

LGTM!

#548 introduced a behavior change on `sdf::Element::FilePath()` for elements loaded from strings. Before that PR, `FilePath()` returned `""` for such elements, but presently, it returns either `data-string` or `<data-string>`. This behavior change could cause failures in downstream applications once released (I know the test `INTEGRATION_save_world` in ign-gazebo will have test failures). Signed-off-by: Addisu Z. Taddese <[email protected]>

github-actions bot added the 🏢 edifice Ignition Edifice label Apr 27, 2021

aaronchongth changed the title ~~Aaron/xpath like trace~~ Encode XML path in Element and add trace in parsing errors Apr 27, 2021

aaronchongth force-pushed the aaron/xpath-like-trace branch from 7a3a599 to 5f437b5 Compare April 27, 2021 07:23

aaronchongth requested a review from azeey April 27, 2021 08:31

azeey requested changes Apr 28, 2021

View reviewed changes

include/sdf/Element.hh Show resolved Hide resolved

include/sdf/Error.hh Outdated Show resolved Hide resolved

src/Utils.cc Show resolved Hide resolved

aaronchongth force-pushed the aaron/xpath-like-trace branch from 1ae7778 to 7c04c40 Compare May 3, 2021 15:19

aaronchongth requested a review from azeey May 3, 2021 17:18

EricCousineau-TRI reviewed May 6, 2021

View reviewed changes

src/Element_TEST.cc Outdated Show resolved Hide resolved

aaronchongth changed the title ~~Encode XML path in Element and add trace in parsing errors~~ Encode XML path and line number in Element and add XML path setter Error. May 7, 2021

azeey requested a review from jennuine May 10, 2021 22:47

jennuine reviewed May 12, 2021

View reviewed changes

include/sdf/Element.hh Outdated Show resolved Hide resolved

include/sdf/Error.hh Show resolved Hide resolved

src/parser.cc Outdated Show resolved Hide resolved

src/parser.cc Outdated Show resolved Hide resolved

jennuine linked an issue May 12, 2021 that may be closed by this pull request

Should add XPath-like traceability to error messages #510

Closed

EricCousineau-TRI approved these changes May 14, 2021

View reviewed changes

aaronchongth added 6 commits May 15, 2021 03:36

Changing Error API to work with generic path instead of file path

2044e47

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

Added Xml path into API of Error

2912649

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

Adding XmlPath to errors up till before includes are handled

437f571

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

Added tests for new API and error messages

734cdcb

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

Fixed FilePath argument name in Error

a6a3d0b

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

Added XML path information related API to Element

d6fc814

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

Reducing number of lines in readXml function

688f425

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

aaronchongth requested review from jennuine and EricCousineau-TRI May 14, 2021 20:08

jennuine requested changes May 15, 2021

View reviewed changes

test/integration/element_tracing.cc Outdated Show resolved Hide resolved

test/integration/element_tracing.cc Outdated Show resolved Hide resolved

test/integration/element_tracing.cc Outdated Show resolved Hide resolved

test/integration/element_tracing.cc Outdated Show resolved Hide resolved

aaronchongth added 2 commits May 15, 2021 09:29

Removing prefixed / for windows CI

9490ed9

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

Removed helper function for clearer failure debugging

ee7f583

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

EricCousineau-TRI approved these changes May 15, 2021

View reviewed changes

Checking against different file path, as it gets loaded as test_model…

bb37c53

…/model.sdf regardless Signed-off-by: Aaron Chong <[email protected]>

aaronchongth requested a review from jennuine May 17, 2021 02:57

jennuine requested changes May 17, 2021

View reviewed changes

test/integration/element_tracing.cc Outdated Show resolved Hide resolved

src/Element_TEST.cc Outdated Show resolved Hide resolved

src/parser.cc Show resolved Hide resolved

Removed lines unrelated to test case

7aa8519

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

azeey approved these changes May 18, 2021

View reviewed changes

aaronchongth added 3 commits May 19, 2021 00:50

Adding minimal path sanitization into findFileCb, reverting to use se…

bd721e9

…parated arguments in sdf::testing::TestFile Signed-off-by: Aaron Chong <[email protected]>

Fix linting errors

feea2f5

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

Removed unused vector include

f90ad09

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

Merge branch 'sdf11' into aaron/xpath-like-trace

1fde2ad

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

aaronchongth requested a review from jennuine May 19, 2021 02:09

aaronchongth enabled auto-merge (squash) May 19, 2021 02:10

jennuine approved these changes May 19, 2021

View reviewed changes

aaronchongth merged commit 7be5c0b into sdf11 May 19, 2021

aaronchongth deleted the aaron/xpath-like-trace branch May 19, 2021 18:10

EricCousineau-TRI mentioned this pull request May 20, 2021

Should associate sdf::Element with XPath #538

Closed

azeey mentioned this pull request Jun 3, 2021

Revert behavior of FilePath() for elements loaded from strings #582

Merged

7 tasks

scpeters mentioned this pull request Jun 28, 2021

Gazebo crashes with Edifice binaries gazebosim/gz-sim#891

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Encode XML path and line number in Element and add XML path setter Error. #548

Encode XML path and line number in Element and add XML path setter Error. #548

aaronchongth commented Apr 27, 2021 •

edited

Loading

codecov-commenter commented Apr 27, 2021 •

edited

Loading

aaronchongth commented Apr 28, 2021

azeey left a comment

aaronchongth commented Apr 29, 2021

EricCousineau-TRI commented May 6, 2021

EricCousineau-TRI commented May 6, 2021

azeey commented May 10, 2021

jennuine left a comment

EricCousineau-TRI commented May 14, 2021

EricCousineau-TRI left a comment

jennuine commented May 14, 2021

EricCousineau-TRI commented May 14, 2021

aaronchongth commented May 14, 2021

aaronchongth commented May 15, 2021

jennuine left a comment

azeey left a comment

aaronchongth commented May 18, 2021

jennuine left a comment

Encode XML path and line number in Element and add XML path setter Error. #548

Encode XML path and line number in Element and add XML path setter Error. #548

Conversation

aaronchongth commented Apr 27, 2021 • edited Loading

🎉 New feature

Summary

Test it

Checklist

codecov-commenter commented Apr 27, 2021 • edited Loading

Codecov Report

aaronchongth commented Apr 28, 2021

azeey left a comment

Choose a reason for hiding this comment

aaronchongth commented Apr 29, 2021

EricCousineau-TRI commented May 6, 2021

EricCousineau-TRI commented May 6, 2021

azeey commented May 10, 2021

jennuine left a comment

Choose a reason for hiding this comment

EricCousineau-TRI commented May 14, 2021

EricCousineau-TRI left a comment

Choose a reason for hiding this comment

jennuine commented May 14, 2021

EricCousineau-TRI commented May 14, 2021

aaronchongth commented May 14, 2021

aaronchongth commented May 15, 2021

jennuine left a comment

Choose a reason for hiding this comment

azeey left a comment

Choose a reason for hiding this comment

aaronchongth commented May 18, 2021

jennuine left a comment

Choose a reason for hiding this comment

aaronchongth commented Apr 27, 2021 •

edited

Loading

codecov-commenter commented Apr 27, 2021 •

edited

Loading