[pkg-apparmor] UDD notifications script

[u]

Hi,

Christian Boltz:
> may I also offer some "nice to have" comments? ;-)

Indeed, thanks a lot!

> Am Donnerstag, 19. Februar 2015 schrieb intrigeri:
[..]
> so maybe (untested!)
>     buglist = cursor.fetchall()
> could work

it does!

>> * When using the get_bug_list output (a list of pairs) in
>>   send_team_notification, we have to access it by indices: bug[0] to
>>   get the bug ID, bug[1] to get the added or removed usertag. It's not
>> astonishingly obvious. I would recommend that get_bug_list outputs:
>>
>>   - either a list of dictionaries so we can then write bug['id']
>>     and bug['usertag']
> 
> I didn't see an obvious way to do this in the psycopg docu, so if we 
> really want this, you'll probably need something like
>     buglist2 = []
>     for bug in buglist:
>         buglist2.append( { 'id': bug[0], 'usertag': bug[1] })
> which is not worth the effort IMHO.
> 
> Either keep the numbers, or define constands like
>     field_id = 0
>     field_usertag = 1
> that you can use instead (define those constants inside each function 
> to avoid conflicting field numbers for another function/query!)

In the end I did that the way intrigeri suggested, but your suggestion
seemed quite straight forward too.

>>   - or a dictionary whose keys would be the bug IDs, and values would
>>     be the usertag; but wait! this would be too limiting, read on:
> 
> Indeed - what happens if a bug has multiple usertags?

If a bug has multiple tags, each one will be a list item, so we can
compare if this one list item already exists.

>> * I see we first retrieve a list of usertagged bugs (in get_bug_list),
>> and then run one more query per bug that had a usertag added or
>> removed, in order to get its title. Alternatively, get_bug_list could
>> use a JOIN to retrieve all the data we need about all usertagged bugs
>> in one go. No idea the higher number of queries (current
>> implementation) costs more than retrieving the title of bugs that we
>> won't use (suggested implementation). 

[..]
> In this case, the question is how many bug ids and tags the first query 
> returns, and for how many of them you need to look up the bug title.
> 
> Anyway, you can speed it up by replacing multiple calls of
>     SELECT title from bugs WHERE id='1'
>     SELECT title from bugs WHERE id='2'
>     SELECT title from bugs WHERE id='3'
> with one call of
>     SELECT id, title from bugs WHERE id IN ('1', '2', '3')
> 
> Note that I included the id in the query to avoid ordering problems.
> 
> Also note that this is the first thing to fix/change if you notice 
> performance problems ;-)

Ok.

> 
> BTW: did I ever point you to my "1001 bugs - or: the golden rules of 
> bad programming" slides? ;-)
> 
> English version:
>     http://blog.cboltz.de/archives/63-1001-bugs-or-the-golden-rules-of-bad-programming.html
> German version:
>     http://blog.cboltz.de/archives/64-1001-Bugs-oder-die-goldenen-Regeln-fuer-schlechte-Programmierung.html
> 
[..]

>> Anyhow, putting aside
>> performance and resources considerations, I would find the
>> implementation of send_team_notification more readable and elegant if
>> get_bug_list returned a list of dictionaries, each such dictionary
>> having keys (id, title, usertag). Food for thought :)
> 
> Can there be multiple usertags on a bug?

yes. And each will produce an own list item.

[...]

> In general I agree, however in this case it would be the only line after 
> the except block, so the current code is probably easier to understand.

Ok. I agree about the readability. Still, in the end I put the return on
the bottom of the function so it would be like for the other functions
in this script which have a similar structure.

> The perfect solution would be:
> 
> def read_statefile(state_filename):
>     statefile_content = ''
>     try:
>         ...
>     except ...:
>         ...
> 
>     return statefile_content
> 
> 
> Final note: I wrote this mail without actually testing anything. 
> Also, I only looked at the parts listed in this mail, not the whole script.

Perfect, it helped me a lot! And it's always great to have 2 points of view.

Cheers
u.