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

Revision history for this message
Guilherme Salgado (salgado) wrote :

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.

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

« Back to merge proposal