Merge lp:~mbp/bzr/initialize into lp:bzr

[Martin Pool]

bzrlib.initialize() now actually initializes, as well as returning a context manager

It's just too perverse that

  import bzrlib
  bzrlib.initialize()

doesn't actually initialize things. This keeps it returning a context manager,
but it's effectively started by default.

[John A Meinel (jameinel)]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 3/31/2011 6:31 AM, Martin Pool wrote:
> Martin Pool has proposed merging lp:~mbp/bzr/initialize into lp:bzr.
>
> Requested reviews:
> Martin Pool (mbp)
> Related bugs:
> Bug #507710 in Bazaar: "want bzrlib.initialize() to do all typical setup"
> https://bugs.launchpad.net/bzr/+bug/507710
>
> For more details, see:
> https://code.launchpad.net/~mbp/bzr/initialize/+merge/55674
>
> It's just too perverse that
>
> import bzrlib
> bzrlib.initialize()
>
> doesn't actually initialize things. This keeps it returning a context manager,
> but it's effectively started by default.

I like the idea, but I don't really see that we have to not use things
the way they are documented. In my test scripts, I certainly do:

 bzrlib.initialize().__enter__()

So this does make things easier there.

I'm not sure that it is perverse to ask people to use the api the way it
was designed. I suppose it is called 'initialize()' and not
'get_initializer_context()'.

Anyway, if you feel it is worthwhile, that's good enough to overcome my
ambivalence.

 merge: approve

John
=:->
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (Cygwin)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iEYEARECAAYFAk2UNxEACgkQJdeBCYSNAAMb7QCfSTWWCzijmHp3x6r1y6CMC5xj
jyAAnircUUtEkXg69yfkvbyNkFZ5fHtJ
=Gjdl
-----END PGP SIGNATURE-----

review: Approve

[Martin Pool (mbp)]

> I like the idea, but I don't really see that we have to not use things
> the way they are documented. In my test scripts, I certainly do:
>
>  bzrlib.initialize().__enter__()
>
> So this does make things easier there.
>
> I'm not sure that it is perverse to ask people to use the api the way it
> was designed. I suppose it is called 'initialize()' and not
> 'get_initializer_context()'.

It's really just the name I think doesn't go with the semantics. As
you say if the name implied that it didn't actually do the
initialization that would be ok.

I wrote this out of annoyance at wondering why it wasn't initializing.
 (I did realize we had the state concept but I thought initialize
automatically entered it.)

Third opinion welcome...

[John Szakmeister (jszakmeister)]

I agree about the name. It gives the impression that you'll be done. I would not have expected a context manager as a result of that call. I like the change.

[Andrew Bennetts (spiv)]

Martin Pool wrote:
[…]
> It's really just the name I think doesn't go with the semantics. As
> you say if the name implied that it didn't actually do the
> initialization that would be ok.
>
> I wrote this out of annoyance at wondering why it wasn't initializing.
> (I did realize we had the state concept but I thought initialize
> automatically entered it.)
>
> Third opinion welcome...

I've made this mistake too, even though I had discussed this API with lifeless
when he was designing/implementing it. I'm in favour of this change.

[Jelmer Vernooij (jelmer)]

On Thu, 2011-03-31 at 08:15 +0000, Martin Pool wrote:
> > I like the idea, but I don't really see that we have to not use things
> > the way they are documented. In my test scripts, I certainly do:
> >
> > bzrlib.initialize().__enter__()
> >
> > So this does make things easier there.
> >
> > I'm not sure that it is perverse to ask people to use the api the way it
> > was designed. I suppose it is called 'initialize()' and not
> > 'get_initializer_context()'.
>
> It's really just the name I think doesn't go with the semantics. As
> you say if the name implied that it didn't actually do the
> initialization that would be ok.
>
> I wrote this out of annoyance at wondering why it wasn't initializing.
> (I did realize we had the state concept but I thought initialize
> automatically entered it.)
I've hit this too. Changing it or the name seems like a good idea. If
neither of us can predict what it does, then it'll be even stranger for
the other external users that should use this API.

Cheers,

Jelmer

[Martin Pool (mbp)]

sent to pqm by email

[Martin Pool (mbp)]

sent to pqm by email

=== modified file 'bzrlib/__init__.py'
--- bzrlib/__init__.py	2011-03-17 17:18:11 +0000
+++ bzrlib/__init__.py	2011-04-01 01:48:32 +0000
@@ -157,13 +157,18 @@
 
     More options may be added in future so callers should use named arguments.
 
+    The object returned by this function can be used as a contex manager
+    through the 'with' statement to automatically shut down when the process
+    is finished with bzrlib.  However (from bzr 2.4) it's not necessary to
+    separately enter the context as well as starting bzr: bzrlib is ready to
+    go when this function returns.
+
     :param setup_ui: If true (default) use a terminal UI; otherwise 
         some other ui_factory must be assigned to `bzrlib.ui.ui_factory` by
         the caller.
     :param stdin, stdout, stderr: If provided, use these for terminal IO;
         otherwise use the files in `sys`.
-    :return: A context manager for the use of bzrlib. The __enter__ method of
-        this context needs to be called before it takes effect, and the __exit__
+    :return: A context manager for the use of bzrlib. The __exit__
         should be called by the caller before exiting their process or
         otherwise stopping use of bzrlib. Advanced callers can use
         BzrLibraryState directly.
@@ -178,7 +183,10 @@
     else:
         ui_factory = None
     tracer = trace.DefaultConfig()
-    return library_state.BzrLibraryState(ui=ui_factory, trace=tracer)
+    state = library_state.BzrLibraryState(ui=ui_factory, trace=tracer)
+    # Start automatically in case people don't realize this returns a context.
+    state._start()
+    return state
 
 
 def test_suite():

=== modified file 'bzrlib/library_state.py'
--- bzrlib/library_state.py	2011-02-08 16:06:21 +0000
+++ bzrlib/library_state.py	2011-04-01 01:48:32 +0000
@@ -31,7 +31,7 @@
     This is the core state needed to make use of bzr. The current instance is
     currently always exposed as bzrlib.global_state, but we desired to move
     to a point where no global state is needed at all.
-    
+
     :ivar saved_state: The bzrlib.global_state at the time __enter__ was
         called.
     :ivar cleanups: An ObjectWithCleanups which can be used for cleanups that
@@ -61,8 +61,16 @@
         """
         self._ui = ui
         self._trace = trace
+        self.started = False
 
     def __enter__(self):
+        if not self.started:
+            self._start()
+        return self # This is bound to the 'as' clause in a with statement.
+
+    def _start(self):
+        """Do all initialization.
+        """        
         # NB: This function tweaks so much global state it's hard to test it in
         # isolation within the same interpreter.  It's not reached on normal
         # in-process run_bzr calls.  If it's broken, we expect that
@@ -86,7 +94,7 @@
 
         self.saved_state = bzrlib.global_state
         bzrlib.global_state = self
-        return self # This is bound to the 'as' clause in a with statement.
+        self.started = True
 
     def __exit__(self, exc_type, exc_val, exc_tb):
         self.cleanups.cleanup_now()

=== modified file 'doc/developers/integration.txt'
--- doc/developers/integration.txt	2011-02-04 05:07:34 +0000
+++ doc/developers/integration.txt	2011-04-01 01:48:32 +0000
@@ -27,9 +27,14 @@
 bzrlib needs ways to handle user input, passwords, a place to emit
 progress bars, logging setup appropriately for your program. The easiest
 way to set all this up in the same fashion ``bzr`` does is to call
-``bzrlib.initialize``. This returns a context manager within which bzrlib
-functions will work correctly. See the pydoc for ``bzrlib.initialize`` for
-more information. In Python 2.4 the ``with`` keyword is not supported and
+``bzrlib.initialize``. 
+
+This returns a context manager within which bzrlib functions will work
+correctly. See the pydoc for ``bzrlib.initialize`` for more information. 
+(You can get away without entering the context manager, because the setup
+work happens directly from ``initialize``.)
+
+In Python 2.4 the ``with`` keyword is not supported and
 so you need to use the context manager manually::
 
   # This sets up your ~/.bzr.log, ui factory and so on and so forth. It is

=== modified file 'doc/en/release-notes/bzr-2.4.txt'
--- doc/en/release-notes/bzr-2.4.txt	2011-03-30 11:50:40 +0000
+++ doc/en/release-notes/bzr-2.4.txt	2011-04-01 01:48:32 +0000
@@ -59,6 +59,12 @@
 * ``Hooks.create_hook`` is now deprecated in favour of ``Hooks.add_hook``.
   (Jelmer Vernooij)
 
+* If you call `bzrlib.initialize` but forget to enter the resulting object
+  as a context manager, bzrlib will now be initialized anyhow.
+  (Previously simple programs calling bzrlib might find the library was
+  mysteriously silent.)
+  (Martin Pool)
+
 Internals
 *********