Code review comment for lp:~manishsinha/zeitgeist/fix-blacklist-api

Revision history for this message
Markus Korn (thekorn) wrote :

I'm exited to see some progress on the blacklist API front, thanks Manish and Seif.
But, to be honest, I don't see which direction this API takes. First of all, in the bug Seif linked we discussed many different approaches, and for me it is not clear which one is implemented here.
And secondly, if I interpret Manish's comment at [0] as final decision (which is perfectly fine with me), I see that the implementation in this branch is completely different

* Changed-signal has signature a(E)s and not sa(E)
* GetTemplates is not returning a dictionary a{sa(E)} but a(sa(E))

Also, when getting a Changed Signal, how do I know what the content means? Is it an added or removed Blacklist?

And one last comment: IMHO pickle sucks, have you considered using JSON, as templates are perfect JSON objects.

[0] https://bugs.launchpad.net/zeitgeist/+bug/612344/comments/18

review: Needs Information

« Back to merge proposal