Code review comment for lp:~salgado/linaro-image-tools/check-hwpack-format

> You mean something like
>
> === modified file 'hwpack/builder.py'
> --- hwpack/builder.py 2010-09-14 18:09:01 +0000
> +++ hwpack/builder.py 2010-11-05 17:20:13 +0000
> @@ -31,6 +31,9 @@
> metadata = Metadata.from_config(
> self.config, self.version, architecture)
> hwpack = HardwarePack(metadata)
> + if ' ' in hwpack.FORMAT:
> + raise AssertionError(
> + 'Format version cannot have white spaces')
> sources = self.config.sources
> hwpack.add_apt_sources(sources)
> fetcher = PackageFetcher(
>
> ?
>
> There's something about it that feels unnatural to me. Maybe it's the
> fact that we're asserting on a class variable, or maybe it's because the
> HardwarePack was just created and the fact that we have to assert right
> after that kind of means we don't have a good contract (as we don't
> really know what we'll get back).
>
> I wouldn't feel that way if we made FORMAT a private attribute (_FORMAT)
> and created a property which asserts it has no space before returning.
> But that sounds like overkill to me as the FORMAT is really a constant
> that should not be changed unless you really know what you're doing.

Right, my concern is the separation between when you change this, and the
actual affect that is has. You can change the FORMAT, and then happily build
hwpacks and not realise the mistake until you go and try and enable that format
in hwpack-install and try and install the hwpack.

Given that you are likely to do both together, and shouldn't be pushing out
hwpacks without testing, I don't think this is that big of a deal, so I'm
happy to leave out this change.

> Maybe we could have just a test asserting that HardwarePack.FORMAT has
> no spaces?

That would work too.

Thanks,

James