-
Notifications
You must be signed in to change notification settings - Fork 50
Add declaration object for deprecation message. #138
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 declaration object for deprecation message. #138
Conversation
Pass the newly captured deprecation information from CastXML to the declarations of those objects.
Codecov Report
@@ Coverage Diff @@
## develop #138 +/- ##
========================================
Coverage 91.68% 91.68%
========================================
Files 136 137 +1
Lines 10613 10671 +58
========================================
+ Hits 9730 9784 +54
- Misses 883 887 +4
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
tom-osika
left a comment
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.
Looks great overall
unittests/test_deprecation.py
Outdated
|
|
||
| def _check_text_content(self, desired_text, comment_decl_string): | ||
| if comment_decl_string: | ||
| self.assertEqual(list, comment_decl_string) |
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.
Should be
| self.assertEqual(list, comment_decl_string) | |
| self.assertEqual(desired_text, comment_decl_string) |
Also, how is this passing CI currently? Comparing the Python token list with comment_decl_string
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.
It's likely due to the fact that the check function is getting an empty string to check against in the version of CastXML that is being used. The if is there to avoid erroring if the test is run against an older version.
I can (and will) update the version that is being downloaded in the CI setup. I also might spend some time making sure those print messages are actually shown to the CI output like some of the other tests have.
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.
@josephsnyder Feel free to re-request a review once the print messages show up. Thanks for updating the CI.
dd27e63 to
1e8df3d
Compare
|
May resolve #122 |
e66f15c to
79d2b9a
Compare
Fix copy/paste copyright header to only be for the year 2021 Fix variable names in the text checking function. Use the latest version of CastXML for testing too.
79d2b9a to
135f8c8
Compare
Pass the newly captured deprecation information from CastXML to the
declarations of those objects.