Add EnforcementPolicy in ParserConfig to enable configurable parsing strictness #481

brawner · 2021-01-30T00:19:03Z

This PR depends on #439, but the PR branch is in a different fork so I can't target it directly.

The added commit is 166b995

This PR adds a configurable WarningsPolicy in ParserConfig to allow users to customize the strictness of the parser. In some cases, a best-effort parse is desired and warnings either streamed to sdfwarn or sdfdbg. But in other cases, like unit tests for example, it makes sense to be pedantic about warnings.

Fixes #473, #327

This is a first approach after discussion over at #473 and I'm totally open to other approaches. Likewise, based on the use cases described in #473 and Bitbucket PR, it might also make sense to add a second configuration option specifically for unrecognized elements.

brawner · 2021-01-30T00:36:14Z

Force pushed 166b995 with a small amount of cleanup.

brawner · 2021-01-30T00:42:33Z

src/parser.cc

               << "], child of element[" << _xml->Value()
               << "], not defined in SDF. Copying[" << elemXml->Value() << "] "
               << "as children of [" << _xml->Value() << "].\n";
+
+        addRecoverableWarning(


Note, for now this changes the default behavior for this specific check back to warn. However, this function takes in a policy enum value so it could be overridden for the default case with something like:

sdf::WarningsPolicy policy = _config.WarningsPolicy(); if (policy != sdf::WarningsPolicy::PEDANTIC) policy = sdf::WarningsPolicy::IGNORED;

It might also be desired to separate WarningsPolicy and add something specific like _config.IgnoreUnrecognizedElements()

Since we've gone back and forth on this behavior in the past, I would vote for having _config.IgnoreUnrecognizedElements() or something similar.

Yah, the more I think about it that makes sense. I switched this back to warn and I'll open a separate PR for easy review of the new additions.

On second thought, after working on it I decided to update this PR. I think there is enough overlap between the two policies that they should have a common enum, which necessitated changes here.

azeey · 2021-02-02T20:57:21Z

src/parser.cc

-      sdfwarn << "Converting a deprecated SDF source[" << _source << "].\n";
+      std::stringstream ss;
+      ss << "Converting a deprecated SDF source[" << _source << "].\n";
+      addRecoverableWarning(


I'm not sure we want to emit an error for reading an older SDFormat file even when the policy is PEDANTIC. The version of readDoc that takes a SDFPtr uses sdfdbg for this.

Switched to sdfdbg

Also, with the new commit this should be easy to add a policy for this in the future should it be desired.

azeey · 2021-02-02T21:35:15Z

test/sdf/unrecognized_elements_with_namespace.sdf

@@ -0,0 +1,11 @@
+<?xml version="1.0" ?>
+<sdf version="1.6">


Can we change the version to 1.8 and use an xmlns attribute to make the xml valid?

Suggested change

<sdf version="1.6">

<sdf version="1.8" xmlns:third_party_software="custom_ns_uri">

azeey · 2021-02-02T21:35:39Z

test/sdf/unrecognized_elements_with_namespace.sdf

@@ -0,0 +1,11 @@
+<?xml version="1.0" ?>
+<sdf version="1.6">
+  <model name="joint_incorrect_tags" some_attribute="staheousth">


nit: what's staheousth?

Just some arbitrary text content. Changed to "some value"

azeey · 2021-02-02T21:39:42Z

src/Utils.hh

@@ -21,8 +21,10 @@
 #include <string>
 #include <optional>
 #include <vector>
+#include "sdf/Console.hh"


nit: Is this needed?

I thought it was for sdfwarn/sdfdbg, but I guess not. Removed

azeey · 2021-02-02T21:41:56Z

src/parser.cc

               << "], child of element[" << _xml->Value()
               << "], not defined in SDF. Copying[" << elemXml->Value() << "] "
               << "as children of [" << _xml->Value() << "].\n";
+
+        addRecoverableWarning(


Since we've gone back and forth on this behavior in the past, I would vote for having _config.IgnoreUnrecognizedElements() or something similar.

brawner · 2021-02-03T19:14:35Z

Err, had to remove the added tests since they will be carried over to the next PR

brawner

After working on a separate commit to handle unrecognized elements, I realized that there was a lot in common with the warnings policy that they should share the same enum, which necessitated some reworking of the naming of different parts.

That said, I think it adds a good framework for adding new recoverable conditions like these in the future.

brawner · 2021-02-03T20:12:29Z

src/parser.cc

               << "], child of element[" << _xml->Value()
               << "], not defined in SDF. Copying[" << elemXml->Value() << "] "
               << "as children of [" << _xml->Value() << "].\n";
+
+        addRecoverableWarning(


On second thought, after working on it I decided to update this PR. I think there is enough overlap between the two policies that they should have a common enum, which necessitated changes here.

brawner · 2021-02-03T20:12:55Z

src/parser.cc

-      sdfwarn << "Converting a deprecated SDF source[" << _source << "].\n";
+      std::stringstream ss;
+      ss << "Converting a deprecated SDF source[" << _source << "].\n";
+      addRecoverableWarning(


Also, with the new commit this should be easy to add a policy for this in the future should it be desired.

azeey

Left a couple of minor comments. Looks great!

src/Utils.cc

src/Utils.hh

azeey · 2021-02-04T21:07:22Z

test/integration/unknown.cc

+    config.SetUnrecognizedElementsPolicy(sdf::EnforcementPolicy::ERR);
+    sdf::Root root;
+    const auto errors = root.Load(testFile, config);
+    EXPECT_FALSE(errors.empty());


Can you add an expectation on at least one of the output messages? Just so it doesn't pass due to other non-related error messages in the future.

azeey · 2021-02-04T21:11:02Z

test/sdf/unrecognized_elements_with_namespace.sdf

@@ -0,0 +1,11 @@
+<?xml version="1.0" ?>
+<sdf version="1.8" xmlns:third_party_software="custom_ns_uri">
+  <model name="joint_incorrect_tags" some_attribute="some value">


nit: joint_incorrect_tags makes it sound this is is not a valid file.

Changed the name to namespaced_tags

Signed-off-by: Stephen Brawner <[email protected]>

scpeters

this is awesome! it shouldn't be in this PR, but I'd love to add a flag to ign sdf commands for setting the parsing level

src/Utils.hh

include/sdf/ParserConfig.hh

Signed-off-by: Stephen Brawner <[email protected]>

scpeters · 2021-02-05T01:49:30Z

this is awesome! it shouldn't be in this PR, but I'd love to add a flag to ign sdf commands for setting the parsing level

#485

brawner · 2021-02-05T23:54:09Z

Thanks for the reviews you two!

brawner requested a review from azeey January 30, 2021 00:19

github-actions bot added the 🏢 edifice Ignition Edifice label Jan 30, 2021

This was referenced Jan 30, 2021

Joint limits silently ignored if improperly nested #473

Closed

//joint/axis/use_parent_model_frame does not fail fast or issue a warning in SDFormat 1.7? #327

Closed

brawner force-pushed the brawner/pedantic-flag branch from ae3cf0d to 166b995 Compare January 30, 2021 00:35

brawner commented Jan 30, 2021

View reviewed changes

azeey reviewed Feb 2, 2021

View reviewed changes

brawner force-pushed the brawner/pedantic-flag branch from e49b501 to f667520 Compare February 3, 2021 19:14

brawner commented Feb 3, 2021

View reviewed changes

brawner force-pushed the brawner/pedantic-flag branch from d0915ea to 9c688dd Compare February 3, 2021 20:17

brawner requested a review from azeey February 3, 2021 20:17

azeey reviewed Feb 4, 2021

View reviewed changes

azeey added this to the SDFormat 1.8 / libsdformat11 milestone Feb 4, 2021

brawner added 5 commits February 4, 2021 15:29

Allow different warning policies in ParserConfig

c08bf36

Signed-off-by: Stephen Brawner <[email protected]>

linters

3d38588

Signed-off-by: Stephen Brawner <[email protected]>

PR fixup

32c2a6c

Signed-off-by: Stephen Brawner <[email protected]>

Switch policies to EnforcmentPolicy for better cohesiveness

bb24a0e

Signed-off-by: Stephen Brawner <[email protected]>

PR Fixup

73a9d98

Signed-off-by: Stephen Brawner <[email protected]>

brawner force-pushed the brawner/pedantic-flag branch from 259b285 to 73a9d98 Compare February 4, 2021 23:35

azeey approved these changes Feb 4, 2021

View reviewed changes

scpeters reviewed Feb 5, 2021

View reviewed changes

src/Utils.hh Outdated Show resolved Hide resolved

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

PR Fixup

8c54ba9

Signed-off-by: Stephen Brawner <[email protected]>

scpeters approved these changes Feb 5, 2021

View reviewed changes

scpeters mentioned this pull request Feb 5, 2021

Add flag to gz sdf to control WarningsPolicy #485

Open

brawner changed the title ~~Add WarningsPolicy in ParserConfig to enable configurable parsing strictness~~ Add EnforcementPolicy in ParserConfig to enable configurable parsing strictness Feb 5, 2021

Merge branch 'master' into brawner/pedantic-flag

4b547fb

brawner merged commit 53491da into master Feb 5, 2021

brawner deleted the brawner/pedantic-flag branch February 5, 2021 23:53

This was referenced Feb 11, 2021

tests: Ensure removed SDFormat elements raise errors in newer versions #490

Merged

proposal: Fail fast on unrecognized elements? #279

Closed

azeey mentioned this pull request Feb 19, 2021

Unknown elements should emit a warning console message instead of a debug message in sdformat.log #409

Closed

EricCousineau-TRI mentioned this pull request Apr 14, 2021

EnforcementPolicy: Shouldn't strictly turn deprecations into errors? #541

Closed

EricCousineau-TRI mentioned this pull request Jun 3, 2021

sdformat: Should exercise newer EnforcementPolicy for deprecation in Drake, and make nominal policy strict RobotLocomotion/drake#15018

Closed

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add EnforcementPolicy in ParserConfig to enable configurable parsing strictness #481

Add EnforcementPolicy in ParserConfig to enable configurable parsing strictness #481

brawner commented Jan 30, 2021 •

edited by azeey

Loading

brawner commented Jan 30, 2021

brawner Jan 30, 2021

azeey Feb 2, 2021

brawner Feb 3, 2021

brawner Feb 3, 2021

azeey Feb 2, 2021

brawner Feb 3, 2021

brawner Feb 3, 2021

azeey Feb 2, 2021

brawner Feb 3, 2021

azeey Feb 2, 2021

brawner Feb 3, 2021

azeey Feb 2, 2021

brawner Feb 3, 2021

azeey Feb 2, 2021

brawner commented Feb 3, 2021

brawner left a comment

brawner Feb 3, 2021

brawner Feb 3, 2021

azeey left a comment

azeey Feb 4, 2021

brawner Feb 4, 2021

azeey Feb 4, 2021

brawner Feb 4, 2021

scpeters left a comment

scpeters commented Feb 5, 2021

brawner commented Feb 5, 2021

	<sdf version="1.6">
	<sdf version="1.8" xmlns:third_party_software="custom_ns_uri">

Add EnforcementPolicy in ParserConfig to enable configurable parsing strictness #481

Add EnforcementPolicy in ParserConfig to enable configurable parsing strictness #481

Conversation

brawner commented Jan 30, 2021 • edited by azeey Loading

brawner commented Jan 30, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brawner commented Feb 3, 2021

brawner left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

azeey left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

scpeters left a comment

Choose a reason for hiding this comment

scpeters commented Feb 5, 2021

brawner commented Feb 5, 2021

brawner commented Jan 30, 2021 •

edited by azeey

Loading