-
-
Notifications
You must be signed in to change notification settings - Fork 74
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
refactor: Remove _dict_update
#980
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #980 +/- ##
==========================================
- Coverage 87.36% 87.35% -0.02%
==========================================
Files 9 9
Lines 4939 4935 -4
==========================================
- Hits 4315 4311 -4
Misses 624 624 ☔ View full report in Codecov by Sentry. 🚨 Try these New Features:
|
param/_utils.py
Outdated
@@ -203,15 +203,6 @@ def _is_mutable_container(value): | |||
return issubclass(type(value), MUTABLE_TYPES) |
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.
Unrelated but this looks more correct to me
return issubclass(type(value), MUTABLE_TYPES) | |
return isinstance(value, MUTABLE_TYPES) |
@@ -203,15 +203,6 @@ def _is_mutable_container(value): | |||
return issubclass(type(value), MUTABLE_TYPES) | |||
|
|||
|
|||
def _dict_update(dictionary, **kwargs): |
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 haven't checked if any downstream uses this.
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 does not look like we use this downstream.
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.
Seems fine to me, though I do always worry if I can't remember why it was done this way originally. Maybe some ancient issue with an earlier Python version?
Either that, it was more complex at some point, or just forgetting that this is possible. |
I think this function does exactly what a normal
dict
does if the first argument is a dict.