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

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

Yes, I wasn't really happy about that. Your solution is much better.

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

I sat there mulling over that for a bit and couldn't see an easy way.
Haven't had a lot of experience with python's testunit.

> [8]
>
> + help="The version template to use when creating the version "
> + "for each ppa."),
>
> Please s/ppa/PPA/ here.

Done.

> This branch is really great, thanks!
No problems.

--
John
Blog http://www.inodes.org
LCA2010 http://www.lca2010.org.nz