-
-
Notifications
You must be signed in to change notification settings - Fork 2k
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
remove DeprecationWarnings from v1 release & fix coverage #2415
Conversation
Codecov Report
@@ Coverage Diff @@
## master #2415 +/- ##
===========================================
+ Coverage 99.90% 100.00% +0.09%
===========================================
Files 25 25
Lines 5099 5080 -19
Branches 1043 1041 -2
===========================================
- Hits 5094 5080 -14
+ Misses 1 0 -1
+ Partials 4 0 -4
Continue to review full report at Codecov.
|
return { | ||
'__dict__': self.__dict__, | ||
'__fields_set__': self.__fields_set__, | ||
'__private_attribute_values__': {k: getattr(self, k, Undefined) for k in self.__private_attributes__}, | ||
'__private_attribute_values__': {k: v for k, v in private_attrs if v is not Undefined}, |
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.
@MrMrRobat I think you wrote this originally, are you happy with the change.
Basically the if value is not Undefined:
below was always True
, I because the behaviour of Undefined
when pickled has changed slightly.
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!
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.
@MrMrRobat do you have any idea why the if value is not Undefined:
line in copy()
now has partial coverage, but used to be covered.
I'm trying to work out, but it makes no sense.
Sorry, I know it's not your problem but it's really confusing me.
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 think it could perhaps be a change in cython, but I'm not sure. Or it could even be a bug fix in codecov? 😕 🤷
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.
Maybe it's because now only present attrs are pickled, while in the past missing attrs used to be pickled as Undefined
? Just a quick assumption, I didn't dig into new code yet.
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.
maybe, it was the same on copy()
.
Hopefully I fixed the coverage without breaking anything 🤞.
* preparing for v1.8 🎉 🚀 * change description for #2415 * tweak change descriptions * fix nested lists in docs * remove items in 1.7.3 from 1.8
To fix coverage before v1.8 #2414.
Most of these warnings should have been removed a long time again.
Removed:
Schema
which was replaced byField
Config.case_insensitive
which was replaced byConfig. case_sensitive
(defaultFalse
)Config.allow_population_by_alias
which was replaced byConfig.allow_population_by_field_name
model.fields
which was replaced bymodel.__fields__
model.to_string()
which was replaced bystr(model)
model.__values__
which was replaced by__dict__