capsul adaptation for populse_db #366
Conversation
sapetnioc
left a comment
sapetnioc
left a comment
There was a problem hiding this comment.
There is a wrong usage of the Storage API here. I believe this code is not working.
| for config in session.configs(module, env): | ||
| id = "%s-%s" % (config._id, env) | ||
| doc = data[session.collection_name(module)][id] | ||
| items = dict(doc._items()) |
There was a problem hiding this comment.
Once get() is called, the value of the document is returned (very likely as a dict in this case). There is no _items() method in Storage API.
There was a problem hiding this comment.
Maybe the test if "config_id" in doc: could be appropriate here ?
There was a problem hiding this comment.
dict() is useless here.
Personally, I would have directly set doc = data[session.collection_name(module)][id].get() and replace items by doc. But this is no important.
sapetnioc
left a comment
sapetnioc
left a comment
There was a problem hiding this comment.
You can merge this PR once you decided what to do (or not to do) with my comments.
| for config in session.configs(module, env): | ||
| id = "%s-%s" % (config._id, env) | ||
| doc = data[session.collection_name(module)][id] | ||
| items = dict(doc._items()) |
There was a problem hiding this comment.
dict() is useless here.
Personally, I would have directly set doc = data[session.collection_name(module)][id].get() and replace items by doc. But this is no important.
I'm doing this little PR because I'm not sure!
I'm still not very comfortable with the new populse_db API and I'm wondering if there might be another, more elegant way of doing things here?
We have the config object by doing:
for config in session.configs(module, env) :I haven't found a way to use this object to retrieve the configuration items, so I'm using the
enumerate()function ...Isn't there a better method?
Isn't regression possible?