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

On Wed, Oct 28, 2009 at 04:36:59AM -0000, Jamu Kakar wrote:
> Review: Approve
> [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.

Done.

>
>
> [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]

Done, with changes in next comment.

> 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)'"

Fixed. It was because it was a different day. I noticed you had some
datetime_factory code in there. I've moved that up into the
BuildTarget itself so that all date related code has access to it.

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

Yes, you should have been CC'd on the email.