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

Revision history for this message
Stefano Rivera (stefanor) wrote :

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

The name "contest" is too board. It should be called programming_contest, if anything.
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 :)

> def setup(self):
> pass

Be aware that if lp:~stefanor/ibid/ibid-plugin-507489 gets in first, it'll clash.

review: Needs Fixing

« Back to merge proposal