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

Add an EnforcementPolicy for deprecated elements #543

Merged
merged 7 commits into from
Apr 29, 2021

Conversation

azeey
Copy link
Collaborator

@azeey azeey commented Apr 21, 2021

🎉 New feature

Closes #541

Summary

If the warning policy of the parser is set to ERR in the sdf::ParserConfig, the parser emits an error message for deprecated elements. This PR adds a separate EnforcementPolicy for deprecated elements so that the behavior of the parser can be configured by the user. The API is such, by default DeprecatedElementsPolicy reflects the value of WarninPolicy, but the user can call SetDeprecatedElementsPolicy to override the value. Note that the order of the function calls does not matter. Once the DeprecatedElementsPolicy overridden, calling WarninPolicy will not change the value of DeprecatedElementsPolicy unless ResetDeprecatedElementsPolicy is called. I'm open to feedback on this API.

Test it

Tests are in test/integration/deprecated_specs.cc

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

🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸

This can be used to configure whether the parser will emit a warning or
an error when encountering a deprecated element.

Signed-off-by: Addisu Z. Taddese <[email protected]>
@azeey azeey self-assigned this Apr 21, 2021
@github-actions github-actions bot added the 🏢 edifice Ignition Edifice label Apr 21, 2021
Copy link
Collaborator

@aaronchongth aaronchongth left a comment

Choose a reason for hiding this comment

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

Looks great! This would be super useful moving forwards.

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 some minor nits, not acutely aligned w/ aim of this PR, so feel free to ignore 'em!

include/sdf/Element.hh Show resolved Hide resolved
test/integration/deprecated_specs.cc Show resolved Hide resolved
test/integration/deprecated_specs.cc Outdated Show resolved Hide resolved
test/integration/deprecated_specs.cc Outdated Show resolved Hide resolved
test/integration/deprecated_specs.cc Outdated Show resolved Hide resolved
azeey and others added 4 commits April 23, 2021 18:06
Co-authored-by: Aaron Chong <[email protected]>
Signed-off-by: Addisu Z. Taddese <[email protected]>
Signed-off-by: Addisu Z. Taddese <[email protected]>
Signed-off-by: Addisu Z. Taddese <[email protected]>
Signed-off-by: Addisu Z. Taddese <[email protected]>
@azeey azeey force-pushed the config_deprecation_error branch from 109dd41 to eebf49b Compare April 23, 2021 23:07
@azeey
Copy link
Collaborator Author

azeey commented Apr 27, 2021

I believe I've addressed all the feedback. @EricCousineau-TRI , PTAL.

@EricCousineau-TRI
Copy link
Collaborator

All of this looks great, thanks!

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.

EnforcementPolicy: Shouldn't strictly turn deprecations into errors?
3 participants