I think referencing the LEP in the docstring is ok but not a substitute for including the docs in the tree - better to include the information in the module so that people don't need a web connection to understand whats going on. I like the intended performance notes - please put them in the docstring so the API docs will show them. I would perhaps say ' 53 + This presents a higher-level interface over the storm model objects, 54 + oriented only towards readers. 55 + 56 + At this level flag names and scope names are presented as strings for 57 + easier use in Python code, though the values remain unicode. They 58 + should always be ascii like Python identifiers. ' As This presents an interface optimised for use in code making conditional choices based on the feature flags. At this level .... This - 165 + # the Launchpad Collection knows how to find a good store to start from, 166 + is a bit weird to mention. 169 === added file 'lib/lp/services/features/scopes.py' 170 --- lib/lp/services/features/scopes.py 1970-01-01 00:00:00 +0000 171 +++ lib/lp/services/features/scopes.py 2010-07-21 21:16:48 +0000 172 @@ -0,0 +1,5 @@ 173 +# Copyright 2010 Canonical Ltd. This software is licensed under the 174 +# GNU Affero General Public License version 3 (see the file LICENSE). 175 + 176 +"""Look up current feature flags. 177 +""" This seems unused - you might want to trim it for now. 179 === added directory 'lib/lp/services/features/tests' 180 === added file 'lib/lp/services/features/tests/__init__.py' 181 --- lib/lp/services/features/tests/__init__.py 1970-01-01 00:00:00 +0000 182 +++ lib/lp/services/features/tests/__init__.py 2010-07-21 21:16:48 +0000 183 @@ -0,0 +1,4 @@ 184 +# Copyright 2010 Canonical Ltd. This software is licensed under the 185 +# GNU Affero General Public License version 3 (see the file LICENSE). 186 + 187 +pass The pass isn't needed :) 189 === added file 'lib/lp/services/features/tests/test_db_settings.py' 190 --- lib/lp/services/features/tests/test_db_settings.py 1970-01-01 00:00:00 +0000 191 +++ lib/lp/services/features/tests/test_db_settings.py 2010-07-21 21:16:48 +0000 192 @@ -0,0 +1,26 @@ 193 +# Copyright 2010 Canonical Ltd. This software is licensed under the 194 +# GNU Affero General Public License version 3 (see the file LICENSE). 195 + 196 +"""Tests for feature settings coming from the database""" 197 + 198 + 199 +from __future__ import with_statement 200 +__metaclass__ = type 201 + 202 +import testtools Launchpad has a TestCase of its own you should use. 204 +from canonical.testing import layers 205 +from lp.services.features.model import ( 206 + FeatureFlag, 207 + FeatureFlagCollection, 208 + ) 209 + 210 + 211 +class TestFeatureModel(testtools.TestCase): 212 + 213 + layer = layers.DatabaseFunctionalLayer 214 + 215 + def test_defaultEmptyCollection(self): 216 + # there are no settings in the sampledata 217 + coll = FeatureFlagCollection() 218 + self.assertTrue(coll.select().is_empty()) I'd make that statement stronger: there should be no settings... 220 === added file 'lib/lp/services/features/tests/test_flags.py' 221 --- lib/lp/services/features/tests/test_flags.py 1970-01-01 00:00:00 +0000 222 +++ lib/lp/services/features/tests/test_flags.py 2010-07-21 21:16:48 +0000 223 @@ -0,0 +1,103 @@ 224 +# Copyright 2010 Canonical Ltd. This software is licensed under the 225 +# GNU Affero General Public License version 3 (see the file LICENSE). 226 + 227 +"""Tests for feature flags. 228 + 229 +""" Personally I prefer single-line docstrings as """fooo ... """ 232 +from __future__ import with_statement 233 +__metaclass__ = type 234 + 235 +import testtools 236 + 237 +from canonical.testing import layers 238 + 239 +from lp.services.features import flags, model 240 + 241 + 242 +notification_name = 'notification.global.text' 243 +notification_value = u'\N{SNOWMAN} stormy Launchpad weather ahead' 244 +example_scope = 'beta_user' 245 + 246 +class TestFeatureFlags(testtools.TestCase): ditto on the test case. 248 + layer = layers.DatabaseFunctionalLayer 249 + 250 + def test_defaultFlags(self): 251 + # the sample db has no flags set 252 + control = flags.FeatureController([]) 253 + self.assertEqual({}, 254 + control.getAllFlags()) 256 + def test_simpleFlags(self): 257 + # with some flags set in the db, you can query them through the 258 + # FeatureController 259 + flag = model.FeatureFlag( 260 + scope=unicode(example_scope), 261 + flag=unicode(notification_name), 262 + value=notification_value, 263 + priority=100) 264 + model.FeatureFlagCollection().store.add(flag) 265 + control = flags.FeatureController(['beta_user']) 266 + self.assertEqual(notification_value, 267 + control.getFlag(notification_name)) One thing to consider is what should be in the controller and what in the model class - I'd say that non tuned-for-conditionals code should be in the model class, so you don't need the controller to have write knowledge: let it be totally tuning and optimised ui. 275 + def test_getAllFlags(self): 276 + # can fetch all the active flags, and it gives back only the 277 + # highest-priority settings .... for a given feature? With that in mind - please land. Note that we're in testfix now so you may need a bit of a gap for that to be fixed.