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

> Serious stuff:
> --------------
>
> What's with the modification to identity? I thought we needed to use a string.

Oops, that slipped through the cleanup I did. r885

> Query: create acm team foo
> ./ibid/plugins/codecontest.py:409: SADeprecationWarning: Use session.add()
> event.session.save_or_update(team)
> Response: Created ACM team foo
> Query: acm team foo
> ERROR:scripts.ibid-plugin:Exception occured in ACMTeamManagement processor of
> codecontest plugin
> Traceback (most recent call last):
> File "scripts/ibid-plugin", line 140, in <module>
> processor.process(event)
> File "./ibid/plugins/__init__.py", line 119, in process
> method(event, *match.groups())
> File "./ibid/plugins/codecontest.py", line 505, in acmteam_info
> u'uva_teams': uva_teams,
> File "./ibid/event.py", line 53, in addresponse
> response = response % params
> ValueError: incomplete format
> WARNING:plugins.unicode:Found a non-unicode string: exception
> Response: I'm not feeling too well

r886

> Stylistic section of the review:
> --------------------------------
>
> > event.addresponse(u'Done')
> All of those kind of things should be replaced with either a more descriptive
> answer ("OK, Team created"), or simply addresponse(True).

r887

> > member = event.session.query(ACMTeamMember) \
> > .filter(ACMTeam.name==team_name) \
> > .filter_by(name=member_name) \
> It'd read better if the two filter lines where the other way around, or you
> used filter() for both instead of one filter(), one filter_by().

r888

> > human_join(sorted([team.name for team in teams])))
> You can use a generator expression (i.e. remove the square brackets)

r889

> > uva_teams = ', and is known as %s on UVa' %
> human_join(uva_teams)
> > else:
> > uva_teams = ''
> Unicode

r890

> > u'%(team_name)s has member%(plural)s %(team_members)s%(uva_teams)s'
> > ...
> > u'plural': plural(len(team.members), '', 's')
> Bad usage of plural. It should be plural(len(team.members), 'member',
> 'members'). There's another one further down.

r891

> > hours = time_penalty / 3600
> > time_penalty -= hours * 3600
> > minutes = time_penalty / 60
> Use the integer division operator //, it makes intent clearer.

r892

> > class ACMProblemScore:
> That should probably be a new-style class. Applies to the others, too.
> Not that it matters :)

r893

> > def __str__(self):
> Should probably be __unicode__, as it returns unicode. Also applies to the
> other class.

r984

> > def setup(self):
> > super(Uva, self).setup()
> Redundant.

r895

> > u"Sorry, I'm not monitoring the scoreboard anyway"
> That needs to be reworded, it doesn't parse.

r896 - 'I never even started monitoring the scoreboard' better?