Code review comment for lp:~rachidbm/ubuntu-l10n-tools/pot-priority

Revision history for this message
David Planella (dpm) wrote :

Looks good, thanks Rachid!

Just a couple of things

* On ul10n_tools/lp_set_pot_priority/__init__.py the "The missing bits from the API are the traversal bits where you'd be able to walk" line and others introduce unnecessary trailing space. Please try to look at the diff before sending future merge proposals
* The new test.csv file has a line with an invalid prioritiy (72a00). Is this intentional?
* It'd be great to start adding automated tests for all tools, and move away this .csv file to be part of the tests. Some background info: https://wiki.ubuntu.com/MeetingLogs/appdevweek1104/RockSolidPython
* I'd suggest moving the PotUtil module somewhere else rather than inside ul10n_tools/lp_set_pot_priority/, as in principle it should be not directly related to setting the priority, but rather in the future it should be able to set any template properties-

These are not blockers, so I'll merge the branch now. Good work!

review: Approve

« Back to merge proposal