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

[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_target can be changed
to:

        if version_template is None:
            version_template = data.get("version_template",
                                        DEFAULT_VERSION_TEMPLATE)

[7]

It would be nice to write a test that ensures that a version
template provided on the command-line overrides a version template
defined in the configuration file, but I suspect that will end up
being rather difficult. I need to spend time improving the
testability of AutoPPA so I wouldn't worry about it for now.

[8]

+ help="The version template to use when creating the version "
+ "for each ppa."),

Please s/ppa/PPA/ here.

This branch is really great, thanks!