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

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

First pass

I'm glad to see you've shortened long lines :)

However, be careful with string types. If you do u'a' 'b', you are concatenating different types of string. That's ok, if ugly, and the result will be unicode, which is what you want. However: r'\a' '\r'. Only the first one is a raw string the second one is a literal CR. I'm talking about diff lines 21x.

== Major ==

62 - autoload = False
63 + #autoload = False

I don't think I need to say anything. And there's more than one.

== Minor ==

44 +from sqlalchemy import Table, Column, ForeignKey
45 +from sqlalchemy.orm import relation
46 +from sqlalchemy.exceptions import InvalidRequestError

I think you can get those from ibid.db

83 + return u'%(user)s (%(usaco_user)s on USACO) %(section)s and ' \
84 + 'last logged in %(days)s ago' % {

Inside brackets (of any kind), you don't need a continuation character (\). There are lots of those.

500 +class Contest:

Newstyle, please.

533 + u'start': self.start.strftime('%d %b %Y %H:%M:%S'),

There's a helper function for formatting times in a bot-wide configurable format.

538 + return mktime(self.start.timetuple()) - mktime(other.start.timetuple())

I think datetimes can be subtracted from each other (giving a timedelta). But obviously this works.

review: Needs Fixing

« Back to merge proposal