Code review comment for lp:~salgado/launchpad/easier-read-only-switch

On IRC, I asked for a comment explaining use of _template suffix for readonly dbs in config.

Correcting this would be nice:

lib/canonical/launchpad/webapp/publication.py
    573: [E1002, LaunchpadBrowserPublication.beginErrorHandlingTransaction] Use super on an old style class

DatabaseConfig.main_master: zcml issue we discussed...

I'm eager to see QA on staging from SAs. Have you discussed with LOSAs how we might test this on staging? I wonder if this would warrant a CP.

I asked you about the first two assertions in TestDatabaseConfig.test_overlay, because I didn't really see the point of them. You said that they look up the values in the wrong config section, which doesn't seem all that compelling to me, but you also said that you had simply moved those from the class' docstring to the new location. I won't worry about them, then.

The following looks like it deserves a bug.

397 + # XXX: Instead of all this we could probably just remove the
398 + # store from ZStorm (zstorm.remove(store)).

Twice you assert that we must use a rw db in a given context. Would it be relatively easy and quick to hint as to why, or do you think that it is quite obvious in context of the full code, rather than the diff?

What would you think of _touch_read_only_file and _remove_read_only_file losing their initial underline and moving to a "testing" module somewhere? They seem to be imported a lot for tests, so they really are a testing API AFAICT, and should be encouraged for (only) that purpose.

Shouldn't lib/canonical/launchpad/readonly.py have an __all__?

In this line from lib/canonical/launchpad/readonly.py...

603 +named read-only.txt under the root of the Launchpad tree.

...I suggest s/under/in/.

In lib/canonical/launchpad/readonly.py, I suggest using os.path.absdir to calculate the ``root`` path just for cleanliness. (There's probably a way to get the path from an environmental var that buildout sets so you don't have to do path funny business, but there's precedent for the funny business, so I'm OK with what you have.)

lib/canonical/launchpad/readonly.py: _currently_in_read_only: is that really part of the utility's interface? It seems like it is an implementation detail.

lib/canonical/launchpad/readonly.py: Just for Python 2.5 fun, why don't you use ``from __future__ import with_statement`` and then ``with self._lock`` instead of the whole try/except acquire/release dance?

I'm submitting these notes now since I am breaking for lunch. More when I return.