Code review comment for lp:~marco-gallotta/ibid/contest

> The name "contest" is too board. It should be called programming_contest, if
> anything.

We still need to agree on a name. I agree that contest is too general. I think programming_contest is too long (20 characters and the longest current name is 12); if we do go with this, I think we should drop the _ to follow the style of other plugins (gameservers, buildbot). Vhata suggested progcomp, but as he said that's ugly. There's also the possibility that maths contests are added: what then?

> Run pyflakes. Otherwise you are simply asking me to do it for you:
>
> ibid/plugins/contest.py:1: 'codecs' imported but unused
> ibid/plugins/contest.py:4: 'ElementTree' imported but unused
> ibid/plugins/contest.py:8: 'ibid' imported but unused
> ibid/plugins/contest.py:10: 'eagerload' imported but unused
> ibid/plugins/contest.py:70: local variable 'etree' is assigned to but never
> used
>
> Also, it's really esoteric, so it should have autoload = False on all the
> Processors.
>
> > priority = -20
> why? Put a comment in explaining please.
>
> > urlencode({u'NAME': user, u'PASSWORD': password})
> URLencode doesn't encode to utf-8, so utf-8 encode the parameters yourself,
> and give it bytestrings.
>
> Applies to all your urlencodes()
>
> > if font.text and font.text.find('Please try again') != -1:
> More pythonic:
> if font.text and u'Please try again' in font.text:
>
> This crops up again in: division = [b.text for b in etree.getiterator(u'b') if
> b.text and b.text.find(usaco_user) != -1][0]
>
> > class UsacoException(Exception):
> > pass
>
> I believe that if you are creating your own exception to carry an exception
> message it should do so and not rely on Exception, as Python 3's Exception
> doesn't have a message. We don't support Python 3 yet, but it's good practice.
> > .filter(and_(
> > Identity.identity == user,
> > Identity.source == event.source)) \
>
> You can save a line by doing:
> .filter(Identity.identity == user) \
> .filter(Identity.source == event.source) \
>
> > usaco_account = [attr.value for attr in account.attributes if attr.name ==
> 'usaco_account']
>
> If you are going to do that, you do want to eager load attributes.
>
> > __redacted__
> I'd prefer [redacted]. It looks nicer :)

Done

> > def setup(self):
> > pass
>
> Be aware that if lp:~stefanor/ibid/ibid-plugin-507489 gets in first, it'll
> clash.

Hopefully someone notices :)