feat(apiclient): add CRUD methods using cattrs
#133
Conversation
gegnew
changed the title
cattrs
gegnew
force-pushed
the
ge/cattrs2
branch
3 times, most recently
from
d6c3a5e to
11bf3c4
Compare
gegnew
force-pushed
the
ge/cattrs2
branch
4 times, most recently
from
b24cd7b to
46b80e8
Compare
| body = [unstructure_and_clean(thing) for thing in entity] | ||
| # group [(class), (path), (body)] | ||
| unzipped = list(zip(*body)) | ||
| # remove duplicates to ensure all entities are of the same type | ||
| (cls, path, body) = (set(unzipped[0]), set(unzipped[1]), unzipped[2]) | ||
|
|
||
| if len(cls) == 1 and len(path) == 1: | ||
| # then post to bulk endpoint | ||
| res = post_and_restructure( | ||
| List[cls.pop()], path.pop(), list(body) # type: ignore | ||
| ) | ||
| else: | ||
| # post each item individually | ||
| res = [post_and_restructure(*thing) for thing in body] |
There was a problem hiding this comment.
Sorry to drag this out. This still seems a bit overly complicated, but maybe it's because I don't know Python well. Ideas:
-
Group
entity's elements byclass, then iterate over that loop even if it's length-1. Think that avoids theif/elsehere and avoids ever iterating over each item. -
Have an explicit
create_manymethod and throw if all entities are not gates. That's the only bulk creation API that we have anyway. Otherwise it's just as easy for the user to callcreatein a loop.
There was a problem hiding this comment.
Went with option 1; let me know what you think
There was a problem hiding this comment.
Bug noted in comment below. I like explicit and simple code so generally lean toward the second option, but am fine with the first if it's tidy.
To make sure I understand: this "create many" logic is only used for creating multiple gates, right?
There was a problem hiding this comment.
One more consideration as far as preparing for the future: we have bulk APIs for updating multiple FCS files and deleting multiple FCS files also. That makes me think (a) explicit verb_many functions are the way to go and (b) these local functions could be moved to file-level functions and likely reused when updating multiple FCS files, at least.
|
|
||
| if isinstance(entity, list): | ||
| body = [unstructure_and_clean(thing) for thing in entity] | ||
| things = defaultdict(list) |
There was a problem hiding this comment.
I don't think this works. list is an argument to the function, but here you're intending to use the list class.
(Needs tests!)
There was a problem hiding this comment.
This is how you're supposed to create a defaultdict, in this case where a non-present key will return a default []
There was a problem hiding this comment.
This is tested in test_gates.py, but actually only in the ge/cattrs-gates branch (since this is only really relevant to gates). Tests aren't really possible until the changes in ge/cattrs-<entity type> land. However, each of those branches do have tests for this method.
There was a problem hiding this comment.
I just pushed a change that uses create_many at a file-level; I'm not sure it's necessarily better, but it also works 😄
There was a problem hiding this comment.
This is how you're supposed to create a
defaultdict, in this case where a non-present key will return a default[]
I'm confused...
>>> from collections import defaultdict
>>> x = [1,2]
>>> defaultdict(list)
defaultdict(<class 'list'>, {})
>>> defaultdict(x)
Traceback (most recent call last):
File "<stdin>", line 1, in <module>
TypeError: first argument must be callable or NoneMoot question now though since that code is gone.
There was a problem hiding this comment.
defaultdict wants a callable, not an instantiated class; this is because it contains a __missing__() method, which is called if you request a key that doesn't exist. So defaultdict(list)['notAKey'] will return list()
gegnew
force-pushed
the
ge/cattrs2
branch
2 times, most recently
from
8579276 to
7148f83
Compare
Depends on #132