Code review comment for lp:~mbp/launchpad/flags

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.