-
Notifications
You must be signed in to change notification settings - Fork 187
fix: response seralize() should counts new properties added by user
#725
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
base: master
Are you sure you want to change the base?
Conversation
|
There might be a related issue here: #586 |
jeremystretch
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.
Thanks @NeverBehave. I've confirmed the fix but have some suggestions to clean up the logic a bit.
pynetbox/core/response.py
Outdated
| init_vals = None | ||
|
|
||
| # Get all field names from _init_cache | ||
| init_cache_keys = {k for k, v in self._init_cache} |
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.
Can we not just reference set(self._init_cache.keys())?
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.
_init_cache is a list a believe, in order to use it as dict, I can wrap it as dict first.
https://stackoverflow.com/questions/6522446/list-of-tuples-to-dictionary
| all_keys = init_cache_keys | obj_keys | ||
|
|
||
| # Create list of tuples for consistency with _init_cache format | ||
| fields_to_serialize = [(k, None) for k in all_keys] |
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.
Does fields_to_serialize need to contain tuples? It looks like we're discarding the second item in each in the for loop below.
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.
The value will be refetched anyway by the for loop below, which discard the value.
I am trying to keep minimal changes while patching this.
Fixes: #708
init=False, combine fields from both the initial API response and the object's current stateINTERNAL_ATTRSconstant