-
Notifications
You must be signed in to change notification settings - Fork 24
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
Object persistence via traits-cereal #21
Conversation
from traits.adaptation.adaptation_manager import ( | ||
adaptation_manager as GLOBAL_ADAPTATION_MANAGER) | ||
|
||
from storables import Child |
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 you separate this from the apptools
imports and make it a relative import?
# (C) Copyright 2014 Enthought, Inc., Austin, TX | ||
# All right reserved. | ||
# | ||
# This file is confidential and NOT open source. Do not distribute. |
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.
Oops
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.
Not sure where this even came from. Never seen it before in my life!
Docstrings and cleanup are coming! |
def save(self, obj, key=None): | ||
if obj is None: | ||
return None | ||
return self._save(key, obj, set()) |
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.
Are you planning to pass something other than set()
in the future?
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'm not sure, honestly. That's part of why it's tucked away. It assumes, for example that all keyed objects are hashable. Maybe that's not safe?
Does this irk you?
I'm really not a fan of the flat blob structure. The result is opaque and unreadable by anything but |
Is the argument that
I'm also not clear on how we would handle versioning using either of these libraries without creating an intermediate representation... which smells like Blob again. Perhaps I just don't know enough about how to squeeze the best mileage out of it, but I'd at least suggest yaml over json. I'm happy to ditch this if it's not going to work, but it seems like these two give us more readability on disk (kind of...) in exchange for not doing the things we care about. Do you see how they could satisfy our use cases (or corran's... or Ioannis/CanopyGeo's) without rewriting them to work more like |
Could you move the demos out into the examples directory? |
Now that I've read the code a bit more closely, I think I agree with @rkern about the flatness of blobs. It would be nice if blobs could maintain the DAG structure of the original object. A shared object could be serialized in place where it is first encountered and stored as a reference after that. Does that seem reasonable? Also, as a general code organization comment, it would be nice if all of the public methods of classes had docstrings. I'm thinking specifically of |
Yes we could leave the first reference to an object in-place and reference it from elsewhere, although as I mentioned elsewhere, we're moving closer and closer to what The other option is to leave it flat at the top level and use h5 links to make it feel like a tree when you're browsing around. That is probably signficantly less work than special casing the first appearance of each serialized object and was on the list of features-to-attempt anyway. If that turns out to be a failure or doesn't work the way we'd like, I'm not super certain |
Closing this PR for now, as there's no demand for this at this time. |
Now's as good a time as any to start kicking the tires.