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