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

Include file path and line number in parsing errors #512

Merged
merged 23 commits into from
Mar 19, 2021

Conversation

aaronchongth
Copy link
Collaborator

@aaronchongth aaronchongth commented Mar 15, 2021

🎉 New feature

Closes #479

Summary

This adds file and line number information to parsing errors, specifically when readXml is called.

Changes to sdf::Error:

  • FilePath and LineNumber is added to the API
  • Added more constructors to sdf::Error to accommodate the above new fields
  • operator<< now handles printouts depending on the availability of FilePath and LineNumber
  • sdf::Error has been pimpl-ized
  • Added tests

Changes to ign.cc:

  • Uses the << operator on sdf::Error when ign sdf -k <FILE>.sdf is called

Changes to readXml:

  • Added source file path as a function parameter, to be used in errors
  • Added file paths and line numbers where necessary in all constructions of sdf::Error
  • Added tests

Test it

source ws/install/setup.bash
./ws/build/sdformat11/src/UNIT_Error_TEST
./ws/build/sdformat11/src/UNIT_parser_TEST

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

@github-actions github-actions bot added the 🏢 edifice Ignition Edifice label Mar 15, 2021
@aaronchongth aaronchongth requested a review from azeey March 15, 2021 08:22
Signed-off-by: Aaron Chong <aaronchongth@gmail.com>
Signed-off-by: Aaron Chong <aaronchongth@gmail.com>
Signed-off-by: Aaron Chong <aaronchongth@gmail.com>
Signed-off-by: Aaron Chong <aaronchongth@gmail.com>
Signed-off-by: Aaron Chong <aaronchongth@gmail.com>
Signed-off-by: Aaron Chong <aaronchongth@gmail.com>
Signed-off-by: Aaron Chong <aaronchongth@gmail.com>
…heck

Signed-off-by: Aaron Chong <aaronchongth@gmail.com>
Signed-off-by: Aaron Chong <aaronchongth@gmail.com>
Signed-off-by: Aaron Chong <aaronchongth@gmail.com>
Signed-off-by: Aaron Chong <aaronchongth@gmail.com>
Signed-off-by: Aaron Chong <aaronchongth@gmail.com>
Signed-off-by: Aaron Chong <aaronchongth@gmail.com>
Signed-off-by: Aaron Chong <aaronchongth@gmail.com>
Signed-off-by: Aaron Chong <aaronchongth@gmail.com>
Signed-off-by: Aaron Chong <aaronchongth@gmail.com>
@aaronchongth
Copy link
Collaborator Author

@osrf-jenkins run test

@chapulina chapulina added the beta Targeting beta release of upcoming collection label Mar 17, 2021
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.

This looks great! I have a few minor comments listed below.

Since there were no diff related to this, I couldn't comment in context, but it would also be nice for errors printed via enforceConfigurablePolicyCondition to contain line numbers. The best way I could think of was changing the enforceConfigurablePolicyCondition function to take an Error object instead of std::string and ErrorCode.

src/Error.cc Outdated Show resolved Hide resolved
include/sdf/Error.hh Outdated Show resolved Hide resolved
src/parser.cc Outdated Show resolved Hide resolved
src/parser.cc Outdated Show resolved Hide resolved
Signed-off-by: Aaron Chong <aaronchongth@gmail.com>
Signed-off-by: Aaron Chong <aaronchongth@gmail.com>
Signed-off-by: Aaron Chong <aaronchongth@gmail.com>
Signed-off-by: Aaron Chong <aaronchongth@gmail.com>
…abs the filename, line number and message from the Error object

Signed-off-by: Aaron Chong <aaronchongth@gmail.com>
@aaronchongth
Copy link
Collaborator Author

This looks great! I have a few minor comments listed below.

Since there were no diff related to this, I couldn't comment in context, but it would also be nice for errors printed via enforceConfigurablePolicyCondition to contain line numbers. The best way I could think of was changing the enforceConfigurablePolicyCondition function to take an Error object instead of std::string and ErrorCode.

That's a super good idea! I added that functionality and some tests here, c0ddc7a

Signed-off-by: Aaron Chong <aaronchongth@gmail.com>
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.

Great work!

@azeey azeey merged commit 2031028 into master Mar 19, 2021
@azeey azeey deleted the aaron/trace_parsing_failure branch March 19, 2021 14:04
@EricCousineau-TRI
Copy link
Collaborator

Woot, thank you!!!

@EricCousineau-TRI
Copy link
Collaborator

@aaronchongth Would it be possible for you to paste a sample of what the end-user now sees w/ the error messages? (just FTR)

@chapulina
Copy link
Contributor

Here's an example using ign-gazebo:

Error [parser.cc:483] Error parsing XML in file [/home/chapulina/ws_edifice/src/ign-gazebo/examples/worlds/shapes.sdf]: Error=XML_ERROR_MISMATCHED_ELEMENT ErrorID=16 (0x10) Line number=209: XMLElement name=pose

This feature has already saved me some time! 🎉

I wasn't sure what the 0x10 was supposed to be though.

@EricCousineau-TRI
Copy link
Collaborator

Sweet, thank you! \cc @SeanCurtis-TRI

0x10 looks like a hex representation of error code ?

@azeey
Copy link
Collaborator

azeey commented Mar 25, 2021

That's probably an xml syntax error from tinyxml, which is different from the error generated by this PR.

@SeanCurtis-TRI
Copy link

Are the line numbers only available upon parsing, or are they available in any other context?

@azeey
Copy link
Collaborator

azeey commented Mar 25, 2021

There's an API for getting the file name and line number from any sdf::Error object.
https://github.com/osrf/sdformat/blob/98c2cb8bd828b75c4ca2b0b73774dd4f2ecf746b/include/sdf/Error.hh#L186-L193

The values are optional because some errors occur in contexts where there is no file.

@SeanCurtis-TRI
Copy link

So, the line numbers are not available from any other class/element? I.e., it'd be nice to get, say, a "Box" and print something along the lines of:

"The Box on line 127 is really friggin' huge. Are you sure you meant that?"

It's particularly relevant when we drop into the XML directly for an SDF extension as opposed to an SDF-native element.

@aaronchongth
Copy link
Collaborator Author

aaronchongth commented Mar 29, 2021

Greetings! Sorry @EricCousineau-TRI, I should have done that in the PR description. Here are some examples of how it looks like

aaron@ws2:~/workspaces/sdf/src/sdformat/test/sdf$ ign sdf -k includes_missing_model.sdf 
Error [SDF.cc:159] Tried to use callback in sdf::findFile(), but the callback is empty.  Did you call sdf::setFindCallback()?
Error Code 13: [/home/aaron/workspaces/sdf/src/sdformat/test/sdf/includes_missing_model.sdf:L6]:  Msg: Unable to find uri[missing_model]

aaron@ws2:~/workspaces/sdf/src/sdformat/test/sdf$ ign sdf -k includes_missing_uri.sdf 
Error Code 4: [/home/aaron/workspaces/sdf/src/sdformat/test/sdf/includes_missing_uri.sdf:L5]:  Msg: <include> element missing 'uri' attribute

"The Box on line 127 is really friggin' huge. Are you sure you meant that?"

@SeanCurtis-TRI, that's a good point. At this point this will not be able to achieve that. This will probably require the line number and path of each XML element to be embedded into each corresponding sdf::Element as the parser goes through it, (correct me if I misunderstood) such that down the road when an sdf::Box is created, it can warn users if something is wrong specifically to this box element in its XML file. Would this be something to consider as a feature extension? 🤔

@SeanCurtis-TRI
Copy link

@aaronchongth You interpreted my question perfectly. I inferred that the described behavior was not available, but it's nice to get official confirmation. I'd certainly vote for line number accessibility as a desirable extension. Not part of this PR, obviously. But some time in the future.

@EricCousineau-TRI
Copy link
Collaborator

Sweet! I think that's encoded here: #226

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
beta Targeting beta release of upcoming collection 🏢 edifice Ignition Edifice
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Parsing failures can be hard to trace?
5 participants