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

Warn by default on unrecognized elements #1096

Merged
merged 3 commits into from
Jul 29, 2022

Conversation

scpeters
Copy link
Member

🎉 New feature

Addresses the concern motivating #1035

Summary

It is common practice to add elements to a released version of the SDFormat spec without any changes to the spec version number. This results in different versions of the SDFormat specification being embedded in different versions of libsdformat. For example, //material/shininess was added to SDFormat 1.7 in libsdformat 9.8.0 and to 1.7-1.9 in libsdformat 12.5.0, but the tag is not recognized by libsdformat 12.4.0. I think this can lead to significant user confusion, especially if model and world files begin to proliferate that use elements that aren't recognized by a version of libsdformat shipped with Ubuntu that does not receive feature updates.

I have been brainstorming how to address this concern by adding patch numbers to the SDFormat specification in #1035, but it's a bit complicated. I think an easier intermediate step would be to generate warnings by default when unrecognized elements are found by the parser, which is what this pull request does.

Test it

$ gz sdf --check test/sdf/unrecognized_elements.sdf
Warning [Utils.cc:129] [/sdf/model[@name="joint_incorrect_tags"][@some_attribute="staheousth"]:/Users/scpeters/ws/sdformat/src/sdformat/test/sdf/unrecognized_elements.sdf:L3]: XML Attribute[some_attribute] in element[model] not defined in SDF.
Warning [Utils.cc:129] [/sdf/model[@name="joint_incorrect_tags"]/link[@name="link"]/not_a_link_element:/Users/scpeters/ws/sdformat/src/sdformat/test/sdf/unrecognized_elements.sdf:L6]: XML Element[not_a_link_element], child of element[link], not defined in SDF. Copying[not_a_link_element] as children of [link].
Warning [Utils.cc:129] [/sdf/model[@name="joint_incorrect_tags"]/not_a_model_element:/Users/scpeters/ws/sdformat/src/sdformat/test/sdf/unrecognized_elements.sdf:L8]: XML Element[not_a_model_element], child of element[model], not defined in SDF. Copying[not_a_model_element] as children of [model].
Warning [Utils.cc:129] [/sdf/not_an_sdf_element:/Users/scpeters/ws/sdformat/src/sdformat/test/sdf/unrecognized_elements.sdf:L10]: XML Element[not_an_sdf_element], child of element[sdf], not defined in SDF. Copying[not_an_sdf_element] as children of [sdf].
Valid.

Checklist

  • Signed all commits for DCO
  • Added tests
  • Added example and/or tutorial
  • Updated documentation (as needed)
  • Updated migration guide (as needed)
  • Consider updating Python bindings (if the library has them)
  • codecheck passed (See contributing)
  • 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 and edit the commit message to match the pull request summary while retaining Signed-off-by messages.

Change the default value of the UnrecognizedElementsPolicy
from LOG to WARN.

Signed-off-by: Steve Peters <scpeters@openrobotics.org>
@scpeters scpeters added the Breaking change Breaks API, ABI or behavior. Must target unstable version. label Jul 28, 2022
@scpeters scpeters requested a review from chapulina July 28, 2022 08:00
@scpeters scpeters requested a review from azeey as a code owner July 28, 2022 08:00
@github-actions github-actions bot added the 🌱 garden Ignition Garden label Jul 28, 2022
@codecov
Copy link

codecov bot commented Jul 28, 2022

Codecov Report

Merging #1096 (648be0e) into main (f262777) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main    #1096   +/-   ##
=======================================
  Coverage   83.16%   83.16%           
=======================================
  Files         154      154           
  Lines       18846    18846           
=======================================
  Hits        15673    15673           
  Misses       3173     3173           
Impacted Files Coverage Δ
src/ParserConfig.cc 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 f262777...648be0e. Read the comment docs.

@chapulina chapulina added the bug Something isn't working label Jul 28, 2022
Copy link
Contributor

@chapulina chapulina left a comment

Choose a reason for hiding this comment

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

LGTM. Downstream applications can always revert back to LOG level to suppress these if they want to.

@scpeters scpeters merged commit e1b270e into main Jul 29, 2022
@scpeters scpeters deleted the scpeters/warn_on_unrecognized_elements branch July 29, 2022 22:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Breaking change Breaks API, ABI or behavior. Must target unstable version. bug Something isn't working 🌱 garden Ignition Garden
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

2 participants