Merge lp://staging/~johnf-inodes/autoppa/version_template into lp://staging/autoppa

Version templates

Not 100% happy with the template variables but I couldn't come up with anything better.

Not 100% happy with the template variables but I couldn't come up with anything better.

Not 100% happy with the template variables but I couldn't come up with anything better.

[1]

+ file = open(filename, "r")
+ try:
+ expected = "version = 'AUTOPPA_VERSION(1.3.11-upside1~dapper~ppa1)'"
+ self.assertEquals(file.read(), expected)
+ finally:
+ file.close()
+ self.builder.version_template = original_version_template

The try/finally isn't really necessary here. If it leaks, the file
will be closed when it gets garbage collected. Also, the final line
isn't necessary since self.builder is created afresh for each test
case.

[2]

+ def _get_version(self, release_name):
+
+ version = self.version_template
+ version = re.sub("\${version}", self.version, version)
+ version = re.sub("\${release_number}", self._get_release_number(release_name), version)
+ version = re.sub("\${release_name}", release_name, version)
+
+ return version

The standard library provides builtin string template interpolation
functionality that will simplify this a bit:

    def _get_version(self, release_name):
        version_template = Template(self.version_template)
        release_number = self._get_release_number(release_name, self.version)
        data = {"version": self.version, "release_name": release_name,
                "release_number": release_number}
        return version_template.substitute(data)

You'll need to add the following import:

from string import Template

[3]

What do you think about expanding the version variables to include
everything in this list:

version 'version' in the diff
revision The revision of the source branch
dist_codename 'release_name' in the diff
dist_version 'release_number' in the diff
now The current time in UTC formatted as YYYYMMDDHHMMSS
today Today's date formatted as YYYYMMDD
user The name from the USER environment variable

Is there anything else that should be included?

[4]

+version_template = ${version}~ppa1~${codename}

+${version} - The version passed on the command line
+${release_number} - 6.04, 6.10, etc
+${release_name} - dapper, gutsy, etc

'codename' should be 'release_name' above, in the README.

[5]

It'd be cool to add a help topic describing the functionality. If
you look at autoppa.help_topics you'll see an example of a topic.
If you implement a class following the same pattern called
topic_version_templates it will be included in the list of topics
you see when you run 'bin/autoppa help topics' and can be viewed by
running 'bin/autoppa help version-templates'.

[6]

+ build_target = self._configuration[build_target]
+ if "version_template" not in build_target:
+ build_target["version_template"] = DEFAULT_VERSION_TEMPLATE
+ return build_target

I think this logic is a bit dangerous, because it will end up
injecting data into user configuration that wasn't actually provided
by the user. I recommend you revert this change and deal with the
default version template in autoppa.application. I would move the
definition of DEFAULT_VERSION_TEMPLATE there, too.

+ version_template = version_template or data["version_template"]

Then the above line in AutoPPA._create_build...

On Thu, Oct 22, 2009 at 07:03:12PM -0000, Jamu Kakar wrote:
> Review: Needs Fixing
> [1]
>
> + file = open(filename, "r")
> + try:
> + expected = "version = 'AUTOPPA_VERSION(1.3.11-upside1~dapper~ppa1)'"
> + self.assertEquals(file.read(), expected)
> + finally:
> + file.close()
> + self.builder.version_template = original_version_template
>
> The try/finally isn't really necessary here. If it leaks, the file
> will be closed when it gets garbage collected. Also, the final line
> isn't necessary since self.builder is created afresh for each test
> case.

OK. I'll fix that up. I pretty much copied this test case from the
one above which does something similar.

> [2]
>
> + def _get_version(self, release_name):
> +
> + version = self.version_template
> + version = re.sub("\${version}", self.version, version)
> + version = re.sub("\${release_number}", self._get_release_number(release_name), version)
> + version = re.sub("\${release_name}", release_name, version)
> +
> + return version
>
> The standard library provides builtin string template interpolation
> functionality that will simplify this a bit:
>
> def _get_version(self, release_name):
> version_template = Template(self.version_template)
> release_number = self._get_release_number(release_name, self.version)
> data = {"version": self.version, "release_name": release_name,
> "release_number": release_number}
> return version_template.substitute(data)
>
> You'll need to add the following import:
>
> from string import Template

Ahh that is very cool. Much more elegant. Thanks!

> [3]
>
> What do you think about expanding the version variables to include
> everything in this list:
>
> version 'version' in the diff
> revision The revision of the source branch
> dist_codename 'release_name' in the diff
> dist_version 'release_number' in the diff
> now The current time in UTC formatted as YYYYMMDDHHMMSS
> today Today's date formatted as YYYYMMDD
> user The name from the USER environment variable
>
> Is there anything else that should be included?

Can't think of anything else right now.
There are a couple of TODOs and FIXMEs regarding this change.

Testing ${revision} is painful since FakeExpect doesn't deal with
grabbing output. I'll implement it after I redo the bzrlib stuff since
then we shouldn't need expect any more.

Also I'm not sure how to stub/mock time and date so that we can test
$today so I left it out for now.

> [4]
>
> +version_template = ${version}~ppa1~${codename}
>
> +${version} - The version passed on the command line
> +${release_number} - 6.04, 6.10, etc
> +${release_name} - dapper, gutsy, etc
>
> 'codename' should be 'release_name' above, in the README.

Done.

> [5]
>
> It'd be cool to add a help topic describing the functionality. If
> you look at autoppa.help_topics you'll see an example of a topic.
> If you implement a class following the same pattern called
> topic_version_templates it will be included in the list of topics
> you see when you r...

OK. All those changes have been pushed

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

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.