[pkg-apparmor] UDD notifications script

[intrigeri]

hi,

intrigeri wrote (18 Feb 2015 16:02:46 GMT) :
>> Would you, Holger and/or intrigeri, care to review this?
>> https://github.com/u451f/usertag-notifications/blob/master/udd.py

> I'll do a code review later today.

Here we go (disclaimer: I'm not a good Python programmer, so I'll have
to nitpick on minor style things and design patterns ;)

Meta: I'm taking this as an opportunity to share with you about
programming in general, because 1. the code apparently works and is
good enough for the task at hand, even without any of the changes I'm
suggesting below! and 2. you and I will soon have more programming
things to do and share on another project, so let's start now to learn
what each of us likes/dislikes/etc., so I'll go into many details.

Feel free to tell me "patches are welcome" on 

* this script needs a shebang, otherwise I've no idea if it's Python 2
  or 3. (isn't it Python 3, btw? if not, why? :)

* s/GPLv=3/GPLv3/

* the "filename" variable could have a less generic name, e.g.
  state_filename; similarly, I would rename the "user" variable to
  "team_email_address", and s/sender/email_sender/ and
  s/receiver/email_receiver/

* s/bdourl/bdo_url/ and s/usertagurl/usertag_url/, for
  readability, perhaps?

* I see:
    # connect to UDD and get all bugs for a certain user
    def uddConnect():
  but it seems that this function only connects to UDD, and the bugs
  retrieval is done elsewhere, so perhaps update the comment?

* Shouldn't we exit with a non-zero error code from errorHandler?
  but then I realize that this function is used both for fatal errors,
  and for mere warnings, so I don't know. Also, errorHandler always
  says "Could not process UDD query", while 2 of its 3 callers have
  nothing to do with UDD queries, which indicates that perhaps this
  function's scope has been extended beyond its initial design, up to
  a point when it needs to be rethought a bit.

* It's a bit confusing that compareState runs saveState, which means
  it doesn't do (only) what its name and documentation say. Same for
  it sending email.

* Generally, I think the functions in this program do too much, rely
  too much on global state, and have too many side effects, which
  makes them harder to (automatically) test in isolation. For example:

  - saveState should take state_filename as a parameter, instead of
    relying on global variables; bugList should take the user as
    a parameter; etc.
  - it would be good to turn compareState into a pure function;
    I guess it would take (old_buglist, new_buglist) and return a list
    of changes, that we would pass to another function whose sole
    purpose would be to send notifications
  - I could go on and on, but you got the idea, right? :)

* Maybe the parameters we pass to the smtplib.SMTP constructor could
  be part of the configuration on top of the script?

* In compareState, maybe rename "data" to "olddata", since we have
  "newdata" already. It would make the algo easier to grasp, at least
  for me.

* compareState seems to only report back about added usertags, but not
  about removed usertags. Should be easy enough to do the latter
  too, no?

* In compareState, apparently we're saving state even in the case when
  we've discarded the new data set as invalid, and decided it's not
  worth comparing it to the old data set. Is it on purpose?
  This wouldn't happen if the errorHandler that was called with "Could
  not retrieve new data" did exit the script.

And, to end with: I'm happy this script exists, runs fine, and does
its job! Congrats :)

Cheers!
-- 
intrigeri