-
Notifications
You must be signed in to change notification settings - Fork 25
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
Serde is now a non-optional dependency #996
Conversation
Removed the "enable-serde" flag and made Serde a non-optional dependency. Removed the cfg attributes that depended on the feature since now the library is included by default.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #996 +/- ##
==========================================
+ Coverage 89.71% 90.01% +0.30%
==========================================
Files 169 168 -1
Lines 23029 22959 -70
==========================================
+ Hits 20660 20667 +7
+ Misses 2369 2292 -77 ☔ View full report in Codecov by Sentry. |
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, thanks! The code coverage issues are probably coming from us not using these structs at all. Would you be able to test which ones are needed by simply deleting them and compiling the code? We may be able to clean up even more things
I went over the classes that were flagged as part of the coverage check and they were either: |
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, will merge it tomorrow
Removed the
enable-serde
flag and made Serde a non-optional dependency. Removed thecfg
attributes that depended on the feature since now the library is included by default.Testing
Both unit tests and
./pre-commit
pass.closes #588