Map annotation enhancement; list of values possible #112
Conversation
| for item in map_annotation: | ||
| if item[0] in map_annotation_dict: | ||
| if not isinstance(map_annotation_dict[item[0]], list): | ||
| map_annotation_dict[item[0]] = [map_annotation_dict[item[0]]] |
There was a problem hiding this comment.
I think this bit might be much simplified by using defaultdicts instead of regular dicts - the downside is that every value from every key-value pair would be a list (of size 1, if there's a single value for that key).
There was a problem hiding this comment.
hmmm, yes.
that is a smoother way to write this.
I was not aware of this feature.
I would even say it makes more sense to get a list for everything, so you always get the same data-type.
Something like:
map_annotation_dict = defaultdict(list)
for item in map_annotation:
map_annotation_dict[item[0]].append(item[1])
But I would leave the decision about this up to you, as it is your library.
There was a problem hiding this comment.
I think using defaultdicts and always getting a list might make more sense (and results in a more consistent API). It is a breaking change for the API, so it might take a bit longer to make it to release, but I think it makes future maintenance easier, so we should probably do that.
| @@ -166,8 +166,7 @@ def set_or_create_screen(conn: BlitzGateway, screen: Union[str, int], | |||
|
|
|||
| def multi_post_map_annotation(conn: BlitzGateway, object_type: str, | |||
| object_ids: Union[int, List[int]], kv_dict: dict, | |||
| ns: str, across_groups: Optional[bool] = True | |||
| ) -> int: | |||
| ns: str) -> int: | |||
There was a problem hiding this comment.
any reason why across_groups has disappeared here?
There was a problem hiding this comment.
Because the parameter was not being used in the function and it did not have the decorator @do_across_groups
I just saw it and removed it.
There was a problem hiding this comment.
ah, good point! I didn't realize this was an importer function.
|
Heyo, |
|
ok, with the benefit of a couple more days to think about this, I think I've flipped back to your original implementation. Most of the time there will be a single value per key, and for that more common use case we should stick to the main tenet of ezomero (a simple interface for the Python API), even if that makes maintenance marginally more difficult - so that should return a dict with string values, instead of always returning a list. Apologies @JensWendt for the extra work and for the flip-flopping :) I've submitted a commit reverting things to your original implementation. Thanks a lot for the PR! |
|
Okay, |
We should all be good to go - just gave it a day so you could see this was happening before I merged it :) |
Description
This tries to implement the option to pass a Key-Value dict containing multiple values for one key which then will create multiple Key-Value pairs in OMERO.
Multiple instances of the key, each with one value.
kv_dict = {"key1": ["value1", "value2"]}would become in OMERO:
The reason for this is, that I needed/wanted it, as the same functionality is also present in the new "KeyValue_from_csv" OMERO.web scripts.
And it doesn't make sense to me, that the python package should have less functionality.
Checklist
It seems to me like I checked everything.
But please double check, as I am not intimately familiar where I have to adapt things everywhere, as not to break things.
For reviewers
later.
__init__.py.