-
Notifications
You must be signed in to change notification settings - Fork 95
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 USDError class #836
Add USDError class #836
Conversation
Signed-off-by: Ashton Larkin <[email protected]>
Codecov Report
@@ Coverage Diff @@
## sdf12 #836 +/- ##
=======================================
Coverage 90.76% 90.76%
=======================================
Files 78 78
Lines 12535 12535
=======================================
Hits 11378 11378
Misses 1157 1157 Continue to review full report at Codecov.
|
From the discussion in #828 (comment), I thought it would be okay to add USD error codes to the sdf ErrorCode enum. While I understand the motivation for placing these error codes in the USD component, I think the issue with that approach is that we'd miss out on the functionality provided by the |
It would nice to separate the USD error codes and put them in the USD component, but I'm not sure know how we can do that and still use |
How about wrapping a usd specific error class in a new But then looking at some of the methods in |
I'm not sure if I understand the approach proposed here - is the idea to add a new enum to ErrorCode that would represent a wrapped error class, and then at runtime, if the error is of this wrapped type, then another method can be called to get the underlying error type? In theory, I think this could work, but I'm not sure if there would be any drawbacks to this approach. @azeey @ahcorde what do you think? Should we stick with 00a3dc6, where the USD error types are listed directly in |
Yeah, thats right. I think that if possible, we should use the usd error directly, and only wrap it in a sdf error when we need to pass it to other components. |
I made |
Signed-off-by: Teo Koon Peng <[email protected]>
@koonpeng I like the approach you prototyped. We might need to be more careful about namespaces since Whether |
@koonpeng, thanks for the prototype - I agree with what @azeey said, this looks pretty good. Unfortunately, there is some code duplication that probably cannot be avoided, but if we want to separate USD errors from SDF errors, this may be the best approach we have at the moment. Regarding namespaces/potential naming collisions, perhaps we can make the following changes to the prototype in
I'm not sure if some of the renaming I proposed above makes sense since these objects are already in the USD namespace (perhaps the |
@koonpeng after some discussion that was had offline, we are going to go with your approach of creating an error class in the USD component and wrapping |
Signed-off-by: Teo Koon Peng <[email protected]>
Signed-off-by: Teo Koon Peng <[email protected]>
Signed-off-by: ahcorde <[email protected]>
@osrf-jenkins retest this please |
Signed-off-by: Ashton Larkin <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just have some minor recommendations for documentation and a repeat of the visibility macro change in #849
Signed-off-by: Ashton Larkin <[email protected]>
All review feedback has been addressed, but I would like to hear what others think about #836 (comment). |
Signed-off-by: Ashton Larkin <[email protected]>
Signed-off-by: Ashton Larkin <[email protected]>
Signed-off-by: Ashton Larkin <[email protected]>
I'm planning to merge this if CI comes back green, unless anyone has any other final review comments. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
Signed-off-by: Ashton Larkin <[email protected]> Co-authored-by: Addisu Z. Taddese <[email protected]>
2b1dffb
to
85962f6
Compare
This pull request has been mentioned on Gazebo Community. There might be relevant details there: https://community.gazebosim.org/t/new-ignition-releases-2022-04-13-fortress-edifice/1367/1 |
Signed-off-by: Ashton Larkin [email protected]
🎉 New feature
Summary
Adding USD-specific error codes that can be used in the USD component: see #828 (comment)
Once this is merged, I will update #828 to use these error codes.
Test it
N/A
Checklist
codecheck
passed (See contributing)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.