-
Notifications
You must be signed in to change notification settings - Fork 1
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
Enable creation of nodes from dictionary #7
base: main
Are you sure you want to change the base?
Conversation
I will merge this into #6 once approved. Keeping the PRs small to handle. |
Questions for @brili while I went through this one. Is there a specific reason why we inherit from built-in |
|
||
# Early exit for initialized nodes | ||
if self.initialized: | ||
return |
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.
you need to assign valid kwargs to self at this point and then return
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.
Did I do it right now?
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.
yes but we need to make sure not to include the extra fields that the api returns, you can refactor the retrieve_by_uuid
and create a separate method from
allowed_data = {}
for key in data:
if key in self.__dict__["schema"]["$defs"][f"{self.__class__.__name__}Post"]["properties"]:
setattr(self, key, data[key])
allowed_data[key] = data[key]
self.__dict__["__original__"] = copy.deepcopy(allowed_data)
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 for the pointers.
I used that snippet to refactor it a little bit.
I think there is some room to streamline this a little bit.
But we can address that in a refactor down the line.
@brili please check if that is along the lines of what you had in mind.
Tests are passing on my end.
(I also see that @izaim added a branch for CI tests here, probably coming soon.)
I am skipping
CriptNode.final_update
if the node is notinitialized
.I added a test that uses Materials from a search, which get successfully converted to nodes.
We may want to try other types of classes too.
I am open to making the new private functions.
CriptNode._from_dict
CriptNode._cls_from_dict
public. I would like to know what you think for the design and user experience.
@Ardi028 for visibility.