-
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
Support batch creating samples #11
Conversation
7d3871b
to
a9c8c86
Compare
@@ -438,11 +439,43 @@ def create(cls, lims, container, position, **kwargs): | |||
|
|||
Udfs can be sent in with the kwarg `udfs`. It should be a dictionary-like. | |||
""" | |||
instance = cls.create_in_memory_instance(lims, container, position, **kwargs) | |||
data = lims.tostring(ElementTree.ElementTree(instance.root)) | |||
instance.root = lims.post(uri=lims.get_uri(cls._URI), data=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.
I think here too that it would be more clear with in_mem_instance and created_instance.
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 in this case, because it's first an in memory instance, but then we make it "more" than that by setting the root. Feels like I would have to rename more in that case.
genologics/entities.py
Outdated
return instance | ||
|
||
@classmethod | ||
def batch_create(cls, lims, instances): |
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.
in_mem_instances?
ret = list() | ||
for entry in response: | ||
uri = entry.attrib['uri'] | ||
ret.append(Sample(lims=lims, uri=uri)) |
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.
Here, it looks like the Sample instance is just initiated with a single uri, and there is another round trip to the db when the Sample instance is to be used. Probably, there is a cache filled up in the .post() call above. I don't like this hidden-ness. I suggest not to care about this in this case :)
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.
Good observation. The reply from the post above actually has no info on the samples, except it knows the URI. Because this call is supposed to be as fast as possible, and it's even rather unlikely that the user wants all the info about the samples created, I think initializing it with the uri is the valid thing to do in this case, (unless we would introduce a parameter like load_all
or similar, so the cost would be explicit to the caller).
a9c8c86
to
a49e72e
Compare
No description provided.