Deprecate changereason #655
Conversation
Codecov Report
@@ Coverage Diff @@
## master #655 +/- ##
==========================================
- Coverage 97.51% 97.11% -0.40%
==========================================
Files 17 17
Lines 925 936 +11
Branches 138 140 +2
==========================================
+ Hits 902 909 +7
- Misses 10 13 +3
- Partials 13 14 +1
Continue to review full report at Codecov.
|
|
|
||
| def get_change_reason_from_object(obj): | ||
| if hasattr(obj, "_change_reason"): | ||
| return getattr(obj, "_change_reason") |
There was a problem hiding this comment.
This could be return obj._change_reason!
(or if this is called many times in loops, even more efficient would be try: return obj.change_reason except AttributeError: pass)
There was a problem hiding this comment.
Agree on the latter - though this will be removed in next version anyway. How could this be return obj._change_reason though? If that doesn't exist, it needs to check changeReason
There was a problem hiding this comment.
I was saying two things at once!
1: not reason to use getattr + string after you’ve already tested that the attribute exists
if hasattr(obj, "_change_reason"):
return obj._change_reason2: if performance matters, you can avoid getting the attribute multiple times (hasattr under the covers tries to get the attr and catches AttributeError):
try:
return object._change_reason
except AttributeError:
pass
# rest of the code that tries changeReasonThere was a problem hiding this comment.
Ah yes, good points.
| " deprecated in 2.10.0 and will be removed in 3.0.0. Use " | ||
| "_change_reason instead. " | ||
| ) | ||
| warnings.warn(warning_msg, DeprecationWarning) |
There was a problem hiding this comment.
warnings most of the time need a stacklevel parameter (usually with value 2, but depends on the calling pattern) so that the message shows the calling code rather that the line with warnings.warn.
There was a problem hiding this comment.
didn't know that. thanks for the feedback
Doesn't make sense for
changeReasonto be camelCase. Replacing it with_change_reasonand deprecating it. Will remove it completely in 3.0