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

Continuing the review notes from the previous comment...

You are using ztapi because other code is doing it (``ztapi.provideUtility(IIsReadOnly, FakeIsReadOnlyUtility())`` in test_authorization.py). ztapi is deprecated in favor of zope.component; it now has all the necessary convenience functions (although the order of arguments for zope.component.provideUtility is reversed IIRC). I'd like to see new code use the "one way to do it".

In lib/canonical/launchpad/readonly.py...

686 + # Only call get_current_browser_request() when we have an interaction,
687 + # or else it will choke.
688 + if queryInteraction() is not None:
689 + request = get_current_browser_request()

...this is a look-before-you-leap approach. Python style guides generally prefers try-except, and I do too. :-)

I question have IIsReadOnly as a utility. Why don't we just have this as a function? The ``_currently_in_read_only`` value (which I said I believe is an implementation detail above) would be a module global--and you already have a mutex to handle modifying that well anyway. Then people would import the function rather than the interface, and things would be more direct. The only downside I can see is that it might be a bit harder to change the behavior of the isReadOnly function for tests--but do you really need to? So in sum, making this a utility doesn't buy us anything AFAICS, and adds extra indirection when a simple function would do.

Do I need to know why/when we use DatabaseBlockedPolicy?

OK, saving these comments; back soon. :-)