Skip to content
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

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

Conversation

InnocentBug
Copy link

@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 not initialized.

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.

  1. CriptNode._from_dict
  2. CriptNode._cls_from_dict

public. I would like to know what you think for the design and user experience.

@Ardi028 for visibility.

@InnocentBug InnocentBug requested a review from brili July 8, 2024 16:29
@InnocentBug InnocentBug self-assigned this Jul 8, 2024
@InnocentBug
Copy link
Author

I will merge this into #6 once approved. Keeping the PRs small to handle.

@InnocentBug
Copy link
Author

Questions for @brili while I went through this one.

Is there a specific reason why we inherit from built-in dict instead of collections.UserDict?
And related, what is the reason that we don't call super() on the parent dict in CriptNode.__init__?
Is that how we make sure certain attributes are ignored?


# Early exit for initialized nodes
if self.initialized:
return
Copy link
Member

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

Copy link
Author

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?

Copy link
Member

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)

Copy link
Author

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.

@InnocentBug InnocentBug requested a review from brili July 10, 2024 15:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants