Code review comment for lp:~johnf-inodes/autoppa/version_template

Revision history for this message
Jamu Kakar (jkakar) wrote :

[9]

+ def _get_revno(self):
+ # FIXME Can't use this until we move to using bzrlib
+ # directly as it causes to much hassle with FakeExpect
+ revno = self.expect.run("bzr revno %s" % self.working_tree,
+ logfile=file,
+ timeout=None)
+ print revno
+ return revno

I would remove this. We can add it when its necessary. Thanks for
adding the TODO about it in _get_version.

[10]

+ "now": time.strftime("%Y%m%d%H%M%S"),
+ "today": datetime.date.today().strftime("%Y%m%d"),

These should be changed to:

        now = datetime.utcnow()
...
            "now": now.strftime("%Y%m%d%H%M%S"),
            "today": now.strftime("%Y%m%d"),

[11]

I have a test failure:

[FAIL]: autoppa.tests.test_target.BuildTargetTest.test_prepare_custom_files_with_custom_version_content

Traceback (most recent call last):
  File "/usr/lib/python2.6/unittest.py", line 279, in run
    testMethod()
  File "/home/jkakar/src/johnf-inodes/autoppa/version-template/autoppa/tests/test_target.py", line 280, in test_prepare_custom_files_with_custom_version_content
    self.assertEquals(file.read(), expected)
exceptions.AssertionError: "version = 'AUTOPPA_VERSION(1.3.11-upside1~dapper~6.06~20091027~user_from_env)'" != "version = 'AUTOPPA_VERSION(1.3.11-upside1~dapper~6.06~20091024~user_from_env)'"

Looking great, +1 considering these comments. Have you had a chance
to sign the Canonical contributor agreement yet?

review: Approve

« Back to merge proposal