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

Revision history for this message
Manish Sinha (मनीष सिन्हा) (manishsinha) wrote :

> 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.

This is the one Mikkel proposed.

> 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)

Yeah. This is just a placement or parameters or dbus signature. Can be changed anytime.

> * GetTemplates is not returning a dictionary a{sa(E)} but a(sa(E))

Are you sure dictionary is a good way to go? I found the latter to be convenient.

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

Same thought from my side. When I was implementing, I was wondering about this. Left this for discussion during the review as the change doesn't involve huge code overhaul.

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

I just carry forwarded what already was there. Didn't change this part. Still, it wont be a huge change in the codebase.

« Back to merge proposal