Merge into bzr.dev : ui-factory : Code : Bazaar

Reviewer	Date Requested	Status
Vincent Ladeuil	2010-09-13	Approve on 2010-09-14
Andrew Bennetts	2010-09-09	Needs Fixing on 2010-09-13
Review via email: mp+34956@code.launchpad.net

Revision history for this message

Vincent Ladeuil (vila) wrote on 2010-09-09:

#

117 + def confirm_action(self, prompt, confirmation_id, args, default=True):

s/args/kwargs/

or even **kwargs like the others, there should be a way to make
this compatible with 'default=True' (which isn't documented nor
used by the way).

Oh wait, yes it *is* used in other implementations, ok.

Hmm, I think that 'confirm' already implies that the default is
True so this may be useless. Do you have a use case for
default=False ?

Adding confirm_action to uis sounds like a good idea, but this
needs to be discussed with the qbzr/bzr-gtk people since they
need a way to override it without calling get_boolean for which
they probably also defines aspecific version...

This may not be a bzrlib problem though...

As mentionned on IRC, I like the direction this is taking
regarding make_test_ui_factory being a good step towards a
fixture independent from the test itself.

Regarding 'bzr.lock.break.confirm', I'm fine with using an
arbitrary name space but sticking to the python module name space
may be better suited in the long term (easier to discover and to maintain).

Now, reagrding the relationships with config variables, we
can (hypothetically):

- define an alias between bzr and bzrlib or
bzr.lock.break.confirm and bzrlib.lock.confirm_break or,
even simpler, between bzrlib.lock.confirm_break and confirm_break_lock

- define (handwaving) a config var as bzrlib.lock.confirm_break
and import it in bzrlib.lockdir

Or any combination.

Anyway, I now better understand what you mean for the application
code and indeed this looks nice.

Making the confirm_action implementation access the relevant
config from the ID may still be a big tricky though (wt, branch
?).

This may be addressed by setting some config attribute to the ui
though... unless we need the context manager for that...

Revision history for this message

Martin Pool (mbp) wrote on 2010-09-10:

#

On 10 September 2010 02:19, Vincent Ladeuil <email address hidden> wrote:
> 117 + def confirm_action(self, prompt, confirmation_id, args, default=True):
>
> s/args/kwargs/
>
> or even **kwargs like the others, there should be a way to make
> this compatible with 'default=True' (which isn't documented nor
> used by the way).

Thanks for the speedy review.

At the moment I really dislike using **kwargs as shorthand for a dict
to be passed to lower level code, eg to be passed in to string
interpolating. It makes things slightly easier on the caller it's
true, but it also tends to break by clashing with other arguments used
either at this level or in different implementations or as the code
changes later on.

> Oh wait, yes it *is* used in other implementations, ok.
>
> Hmm, I think that 'confirm' already implies that the default is
> True so this may be useless. Do you have a use case for
> default=False ?

That would imply an operation where we want confirmation from a user
and we won't proceed in non-interactive mode, which is conceivable but
unlikely. So perhaps I should just delete it.

>
> Adding confirm_action to uis sounds like a good idea, but this
> needs to be discussed with the qbzr/bzr-gtk people since they
> need a way to override it without calling get_boolean for which
> they probably also defines aspecific version...
>
> This may not be a bzrlib problem though...

I'll cc them and they may have a thought.

> As mentionned on IRC, I like the direction this is taking
> regarding make_test_ui_factory being a good step towards a
> fixture independent from the test itself.
>
> Regarding 'bzr.lock.break.confirm', I'm fine with using an
> arbitrary name space but sticking to the python module name space
> may be better suited in the long term (easier to discover and to maintain).

So bzrlib.lockdir.break, or something similar?

> Now, reagrding the relationships with config variables, we
> can (hypothetically):
>
> - define an alias between bzr and bzrlib or
> bzr.lock.break.confirm and bzrlib.lock.confirm_break or,
> even simpler, between bzrlib.lock.confirm_break and confirm_break_lock
>
> - define (handwaving) a config var as bzrlib.lock.confirm_break
> and import it in bzrlib.lockdir
>
> Or any combination.
>
> Anyway, I now better understand what you mean for the application
> code and indeed this looks nice.
>
> Making the confirm_action implementation access the relevant
> config from the ID may still be a big tricky though (wt, branch
> ?).
>
> This may be addressed by setting some config attribute to the ui
> though... unless we need the context manager for that...

Interesting point. istm that you might want per-context configuration
to affect the ui in more than just confirmations...

So is the overall vote here 'tweak, plus check with gui maintainers,
plus update the other callers'?

--
Martin

On 10 September 2010 02:19, Vincent Ladeuil <v.ladeuil+lp@free.fr> wrote:
> 117     + def confirm_action(self, prompt, confirmation_id, args, default=True):
>
> s/args/kwargs/
>
> or even **kwargs like the others, there should be a way to make
> this compatible with 'default=True' (which isn't documented nor
> used by the way).

Thanks for the speedy review.

At the moment I really dislike using **kwargs as shorthand for a dict
to be passed to lower level code, eg to be passed in to string
interpolating.  It makes things slightly easier on the caller it's
true, but it also tends to break by clashing with other arguments used
either at this level or in different implementations or as the code
changes later on.

> Oh wait, yes it *is* used in other implementations, ok.
>
> Hmm, I think that 'confirm' already implies that the default is
> True so this may be useless. Do you have a use case for
> default=False ?

That would imply an operation where we want confirmation from a user
and we won't proceed in non-interactive mode, which is conceivable but
unlikely.  So perhaps I should just delete it.

>
> Adding confirm_action to uis sounds like a good idea, but this
> needs to be discussed with the qbzr/bzr-gtk people since they
> need a way to override it without calling get_boolean for which
> they probably also defines aspecific version...
>
> This may not be a bzrlib problem though...

I'll cc them and they may have a thought.

> As mentionned on IRC, I like the direction this is taking
> regarding make_test_ui_factory being a good step towards a
> fixture independent from the test itself.
>
> Regarding 'bzr.lock.break.confirm', I'm fine with using an
> arbitrary name space but sticking to the python module name space
> may be better suited in the long term (easier to discover and to maintain).

So bzrlib.lockdir.break, or something similar?

> Now, reagrding the relationships with config variables, we
> can (hypothetically):
>
> - define an alias between bzr and bzrlib or
>  bzr.lock.break.confirm and bzrlib.lock.confirm_break or,
>  even simpler, between bzrlib.lock.confirm_break and confirm_break_lock
>
> - define (handwaving) a config var as bzrlib.lock.confirm_break
>  and import it in bzrlib.lockdir
>
> Or any combination.
>
> Anyway, I now better understand what you mean for the application
> code and indeed this looks nice.
>
> Making the confirm_action implementation access the relevant
> config from the ID may still be a big tricky though (wt, branch
> ?).
>
> This may be addressed by setting some config attribute to the ui
> though... unless we need the context manager for that...

Interesting point.  istm that you might want per-context configuration
to affect the ui in more than just confirmations...

So is the overall vote here 'tweak, plus check with gui maintainers,
plus update the other callers'?

-- 
Martin

Revision history for this message

Vincent Ladeuil (vila) wrote on 2010-09-10:

#

Download full text (4.0 KiB)

>>>>> Martin Pool <email address hidden> writes:

    > On 10 September 2010 02:19, Vincent Ladeuil <email address hidden> wrote:
    >> 117 + def confirm_action(self, prompt, confirmation_id, args, default=True):
    >>
    >> s/args/kwargs/
    >>
    >> or even **kwargs like the others, there should be a way to make
    >> this compatible with 'default=True' (which isn't documented nor
    >> used by the way).

> Thanks for the speedy review.

    > At the moment I really dislike using **kwargs as shorthand for a dict
    > to be passed to lower level code, eg to be passed in to string
    > interpolating. It makes things slightly easier on the caller it's
    > true, but it also tends to break by clashing with other arguments used
    > either at this level or in different implementations or as the code
    > changes later on.

I see. Well, turning the callerrs x=2, y=3 into dict(x=2, y=3) won't
such a problem. But in this case, the migration from one to the other
should occur sooner rather than later, keeping different idioms in the
code base... is painful in the long run.

    >> Oh wait, yes it *is* used in other implementations, ok.
    >>
    >> Hmm, I think that 'confirm' already implies that the default is
    >> True so this may be useless. Do you have a use case for
    >> default=False ?

    > That would imply an operation where we want confirmation from a user
    > and we won't proceed in non-interactive mode, which is conceivable but
    > unlikely. So perhaps I should just delete it.

Then we could introduce deny_action when needed ?

    >>
    >> Adding confirm_action to uis sounds like a good idea, but this
    >> needs to be discussed with the qbzr/bzr-gtk people since they
    >> need a way to override it without calling get_boolean for which
    >> they probably also defines aspecific version...
    >>
    >> This may not be a bzrlib problem though...

> I'll cc them and they may have a thought.

    >> As mentionned on IRC, I like the direction this is taking
    >> regarding make_test_ui_factory being a good step towards a
    >> fixture independent from the test itself.
    >>
    >> Regarding 'bzr.lock.break.confirm', I'm fine with using an
    >> arbitrary name space but sticking to the python module name space
    >> may be better suited in the long term (easier to discover and to maintain).

> So bzrlib.lockdir.break, or something similar?

At least internally, but the caller itself may use an aliased form.

    >> Now, reagrding the relationships with config variables, we
    >> can (hypothetically):
    >>
    >> - define an alias between bzr and bzrlib or
    >> bzr.lock.break.confirm and bzrlib.lock.confirm_break or,
    >> even simpler, between bzrlib.lock.confirm_break and confirm_break_lock
    >>
    >> - define (handwaving) a config var as bzrlib.lock.confirm_break
    >> and import it in bzrlib.lockdir
    >>
    >> Or any combination.
    >>
    >> Anyway, I now better understand what you mean for the application
    >> code and indeed this looks nice.
    >>
    >> Making the confirm_action implementation access the relevant
    >> config from the ID may still be a big...

>>>>> Martin Pool <mbp@sourcefrog.net> writes:

> On 10 September 2010 02:19, Vincent Ladeuil <v.ladeuil+lp@free.fr> wrote:
    >> 117     + def confirm_action(self, prompt, confirmation_id, args, default=True):
    >> 
    >> s/args/kwargs/
    >> 
    >> or even **kwargs like the others, there should be a way to make
    >> this compatible with 'default=True' (which isn't documented nor
    >> used by the way).

> Thanks for the speedy review.

> At the moment I really dislike using **kwargs as shorthand for a dict
    > to be passed to lower level code, eg to be passed in to string
    > interpolating.  It makes things slightly easier on the caller it's
    > true, but it also tends to break by clashing with other arguments used
    > either at this level or in different implementations or as the code
    > changes later on.

I see. Well, turning the callerrs x=2, y=3 into dict(x=2, y=3) won't
such a problem. But in this case, the migration from one to the other
should occur sooner rather than later, keeping different idioms in the
code base... is painful in the long run.

>> Oh wait, yes it *is* used in other implementations, ok.
    >> 
    >> Hmm, I think that 'confirm' already implies that the default is
    >> True so this may be useless. Do you have a use case for
    >> default=False ?

> That would imply an operation where we want confirmation from a user
    > and we won't proceed in non-interactive mode, which is conceivable but
    > unlikely.  So perhaps I should just delete it.

Then we could introduce deny_action when needed ?
 
    >> 
    >> Adding confirm_action to uis sounds like a good idea, but this
    >> needs to be discussed with the qbzr/bzr-gtk people since they
    >> need a way to override it without calling get_boolean for which
    >> they probably also defines aspecific version...
    >> 
    >> This may not be a bzrlib problem though...

> I'll cc them and they may have a thought.

>> As mentionned on IRC, I like the direction this is taking
    >> regarding make_test_ui_factory being a good step towards a
    >> fixture independent from the test itself.
    >> 
    >> Regarding 'bzr.lock.break.confirm', I'm fine with using an
    >> arbitrary name space but sticking to the python module name space
    >> may be better suited in the long term (easier to discover and to maintain).

> So bzrlib.lockdir.break, or something similar?

At least internally, but the caller itself may use an aliased form.

>> Now, reagrding the relationships with config variables, we
    >> can (hypothetically):
    >> 
    >> - define an alias between bzr and bzrlib or
    >>  bzr.lock.break.confirm and bzrlib.lock.confirm_break or,
    >>  even simpler, between bzrlib.lock.confirm_break and confirm_break_lock
    >> 
    >> - define (handwaving) a config var as bzrlib.lock.confirm_break
    >>  and import it in bzrlib.lockdir
    >> 
    >> Or any combination.
    >> 
    >> Anyway, I now better understand what you mean for the application
    >> code and indeed this looks nice.
    >> 
    >> Making the confirm_action implementation access the relevant
    >> config from the ID may still be a big tricky though (wt, branch
    >> ?).
    >> 
    >> This may be addressed by setting some config attribute to the ui
    >> though... unless we need the context manager for that...

> Interesting point.  istm that you might want per-context configuration
    > to affect the ui in more than just confirmations...

Yup, hence 'setting some config attribute to the ui' not 'add a
parameter to confirm_action'. There is certainly some risks down that
path regarding getting the setting correct (if some method set the
config to repo but is also called when the config is set to wt should it
override or not ?) but it's too vague in my mind to really know yet.

> So is the overall vote here 'tweak, plus check with gui maintainers,
    > plus update the other callers'?

Oh, you said: "Before landing I want to update the callers but I'd
appreciate feedback now on the api." so I was waiting for the real
submission to review :-)

Revision history for this message

Vincent Ladeuil (vila) wrote on 2010-09-10:

#

116 + def confirm_action(self, prompt, confirmation_id, prompt_kwargs):

Grr, sorry to notice this so late, but isn't:
confirm_action(self, confirmation_id, prompt,prompt_kwargs):

more explicit (your docstring and dev doc focuses on the confirmation_id
which is also a good hint) ?
I.e. the confirmation id, followed by the prompt and its args ?

And regarding check with gui maintainers, yes.

Revision history for this message

Martin Pool (mbp) wrote on 2010-09-10:

#

> > At the moment I really dislike using **kwargs as shorthand for a dict
> > to be passed to lower level code, eg to be passed in to string
> > interpolating. It makes things slightly easier on the caller it's
> > true, but it also tends to break by clashing with other arguments used
> > either at this level or in different implementations or as the code
> > changes later on.
>
> I see. Well, turning the callerrs x=2, y=3 into dict(x=2, y=3) won't
> such a problem. But in this case, the migration from one to the other
> should occur sooner rather than later, keeping different idioms in the
> code base... is painful in the long run.

If you're talking about going back to clean up all existing cases like this, then it is a bit painful. I think not adding any more of them is worthwhile.

I think the coding guideline would be, don't mix kwargs where the caller could want provide any arbitrary value with other parameters. Use kwargs only for metaprogramming. Perhaps this is too harsh and needlessly closes off a useful tool though.

> >> Oh wait, yes it *is* used in other implementations, ok.
> >>
> >> Hmm, I think that 'confirm' already implies that the default is
> >> True so this may be useless. Do you have a use case for
> >> default=False ?
>
> > That would imply an operation where we want confirmation from a user
> > and we won't proceed in non-interactive mode, which is conceivable but
> > unlikely. So perhaps I should just delete it.
>
> Then we could introduce deny_action when needed ?

If we think we'll want it, perhaps we should keep the option now?

> > So bzrlib.lockdir.break, or something similar?
>
> At least internally, but the caller itself may use an aliased form.

I think having aliases for this might just cause confusion. My updated branch shows some suggested names.

Revision history for this message

Gary van der Merwe (garyvdm) wrote on 2010-09-10:

#

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

On 09/09/2010 18:19, Vincent Ladeuil wrote:
> Adding confirm_action to uis sounds like a good idea, but this
> needs to be discussed with the qbzr/bzr-gtk people since they
> need a way to override it without calling get_boolean for which
> they probably also defines aspecific version...

The patch seems cool to me. I can override both got_boolean, and
confirm_action. It would be be cool if confirm_action showed a dialog
with a "Don't ask me this again." checkbox.

review:approve
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.10 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iEYEARECAAYFAkyJ3n4ACgkQd/3EdwGKOh0IWACeKcVRS78gXrqz8Akd0SH/U+zz
NrEAnivrpxukezT+t906ayEYiq3ZGKGF
=MM58
-----END PGP SIGNATURE-----

Revision history for this message

Vincent Ladeuil (vila) wrote on 2010-09-10:

#

>>>>> Martin Pool <email address hidden> writes:

    >> > At the moment I really dislike using **kwargs as shorthand for a dict
    >> > to be passed to lower level code, eg to be passed in to string
    >> > interpolating. It makes things slightly easier on the caller it's
    >> > true, but it also tends to break by clashing with other arguments used
    >> > either at this level or in different implementations or as the code
    >> > changes later on.
    >>
    >> I see. Well, turning the callerrs x=2, y=3 into dict(x=2, y=3) won't
    >> such a problem. But in this case, the migration from one to the other
    >> should occur sooner rather than later, keeping different idioms in the
    >> code base... is painful in the long run.

    > If you're talking about going back to clean up all existing cases
    > like this, then it is a bit painful. I think not adding any more
    > of them is worthwhile.

It will be painful *once*, but less painful for new contributors that
won't wonder which one to use, for reviewers who won't have to say "you
picked the wrong one", etc. Just saying ;)

<snip/>

    >> > That would imply an operation where we want confirmation from a user
    >> > and we won't proceed in non-interactive mode, which is conceivable but
    >> > unlikely. So perhaps I should just delete it.
    >>
    >> Then we could introduce deny_action when needed ?

> If we think we'll want it, perhaps we should keep the option now?

confirm_action(default=False) sounds really akward compared to
deny_action()...

    >> > So bzrlib.lockdir.break, or something similar?
    >>
    >> At least internally, but the caller itself may use an aliased form.

> I think having aliases for this might just cause confusion. My
> updated branch shows some suggested names.

Could be, let's see how it's used (but we already have many conf vars
that could benefit to be associated with a python name and then we'll
have to ensure backward compatibility anyway).

>>>>> Martin Pool <mbp@sourcefrog.net> writes:

>> > At the moment I really dislike using **kwargs as shorthand for a dict
    >> > to be passed to lower level code, eg to be passed in to string
    >> > interpolating.  It makes things slightly easier on the caller it's
    >> > true, but it also tends to break by clashing with other arguments used
    >> > either at this level or in different implementations or as the code
    >> > changes later on.
    >> 
    >> I see. Well, turning the callerrs x=2, y=3 into dict(x=2, y=3) won't
    >> such a problem. But in this case, the migration from one to the other
    >> should occur sooner rather than later, keeping different idioms in the
    >> code base... is painful in the long run.

> If you're talking about going back to clean up all existing cases
    > like this, then it is a bit painful.  I think not adding any more
    > of them is worthwhile.

It will be painful *once*, but less painful for new contributors that
won't wonder which one to use, for reviewers who won't have to say "you
picked the wrong one", etc. Just saying ;)

<snip/>

>> > That would imply an operation where we want confirmation from a user
    >> > and we won't proceed in non-interactive mode, which is conceivable but
    >> > unlikely.  So perhaps I should just delete it.
    >> 
    >> Then we could introduce deny_action when needed ?

> If we think we'll want it, perhaps we should keep the option now?

confirm_action(default=False) sounds really akward compared to
deny_action()...

>> > So bzrlib.lockdir.break, or something similar?
    >> 
    >> At least internally, but the caller itself may use an aliased form.

> I think having aliases for this might just cause confusion.  My
    > updated branch shows some suggested names.

Could be, let's see how it's used (but we already have many conf vars
that could benefit to be associated with a python name and then we'll
have to ensure backward compatibility anyway).

Revision history for this message

Martin Pool (mbp) wrote on 2010-09-13:

#

On 10 September 2010 18:43, Vincent Ladeuil <email address hidden> wrote:
>>>>>> Martin Pool <email address hidden> writes:
>
> >> > At the moment I really dislike using **kwargs as shorthand for a dict
> >> > to be passed to lower level code, eg to be passed in to string
> >> > interpolating. It makes things slightly easier on the caller it's
> >> > true, but it also tends to break by clashing with other arguments used
> >> > either at this level or in different implementations or as the code
> >> > changes later on.
> >>
> >> I see. Well, turning the callerrs x=2, y=3 into dict(x=2, y=3) won't
> >> such a problem. But in this case, the migration from one to the other
> >> should occur sooner rather than later, keeping different idioms in the
> >> code base... is painful in the long run.
>
> > If you're talking about going back to clean up all existing cases
> > like this, then it is a bit painful. I think not adding any more
> > of them is worthwhile.
>
> It will be painful *once*, but less painful for new contributors that
> won't wonder which one to use, for reviewers who won't have to say "you
> picked the wrong one", etc. Just saying ;)

Let's see if we can work out a clear expression of the code guideline.
I don't want to ban kwargs altogether, but I think it's bad to
promise people can pass arbitrary dicts through to a lower level (eg a
format string), when the function also takes other arguments. You
should instead pass a dict.

--
Martin

Revision history for this message

Andrew Bennetts (spiv) wrote on 2010-09-13:

#

Regarding this patch: I'm in favour of it. There's a nit that default=True still exists in the signature of one of the implementations, so Needs Fixing.

I'm not worried about the slight inconvenience of an explicit dict rather than **kwargs. There just aren't enough places where we ask users to confirm an action for this to be a big issue. And I do like the explicit separation of function params vs. string interpolation params.

Regarding the general guidelines for kwargs: I think it should be used sparingly. I think it can be good when a function is delegating largely to another callable (e.g. it is some sort of higher order function, like TestCase.addCleanup). Otherwise I tend to be sceptical. If the parameters to a function are a bit complex and might vary over time (e.g. the commit API) then I think what's really called for is an explicit object for the parameters (e.g. I think we should create a CommitParams object), rather than a bag of positional and/or keyword args. If you have an arbitrary set of keys and values that are different with each use (e.g. string interpolation inputs) then again I think that should not be mixed in with the regular positional/keyword args, it seems like a different category of thing.

I guess a short rule-of-thumb is: be wary of any **kwargs that aren't accompanied by a param called 'callable'.

review: Needs Fixing

Revision history for this message

Martin Pool (mbp) wrote on 2010-09-13:

#

thanks, I folded that text in to
https://code.edge.launchpad.net/~mbp/bzr/doc/+merge/35243

Revision history for this message

Martin Pool (mbp) wrote on 2010-09-13:

#

This now fixes up all the immediately obvious callers of get_boolean.

We could do more as far as configuration etc but I think it's good to land.

Revision history for this message

Vincent Ladeuil (vila) wrote on 2010-09-14:

#

Great !

review: Approve

Revision history for this message

Martin Pool (mbp) wrote on 2010-09-14:

#

sent to pqm by email

Revision history for this message

Martin Pool (mbp) wrote on 2010-09-15:

#

sent to pqm by email

Revision history for this message

Martin Pool (mbp) wrote on 2010-09-15:

#

sent to pqm by email

Revision history for this message

Martin Pool (mbp) wrote on 2010-09-15:

#

sent to pqm by email

Bazaar

Merge lp:~mbp/bzr/ui-factory into lp:bzr

Commit message

Description of the change

Preview Diff

Subscribers

 === modified file 'bzrlib/builtins.py'
 --- bzrlib/builtins.py	2010-09-14 09:29:57 +0000
 +++ bzrlib/builtins.py	2010-09-15 10:43:44 +0000
@@ -4801,8 +4801,11 @@
              self.outf.write('The above revision(s) will be removed.\n')
          if not force:
--            if not ui.ui_factory.get_boolean('Are you sure'):
--                self.outf.write('Canceled')
++            if not ui.ui_factory.confirm_action(
++                    'Uncommit these revisions',
++                    'bzrlib.builtins.uncommit',
++                    {}):
++                self.outf.write('Canceled\n')
                  return 0
          mutter('Uncommitting from {%s} to {%s}',
 === modified file 'bzrlib/lockdir.py'
 --- bzrlib/lockdir.py	2010-09-13 02:32:44 +0000
 +++ bzrlib/lockdir.py	2010-09-15 10:43:44 +0000
@@ -358,7 +358,9 @@
              return
          if holder_info is not None:
              lock_info = '\n'.join(self._format_lock_info(holder_info))
--            if bzrlib.ui.ui_factory.get_boolean("Break %s" % lock_info):
++            if bzrlib.ui.ui_factory.confirm_action(
++                "Break %(lock_info)s", 'bzrlib.lockdir.break',
++                dict(lock_info=lock_info)):
                  self.force_break(holder_info)
      def force_break(self, dead_holder_info):
 === modified file 'bzrlib/msgeditor.py'
 --- bzrlib/msgeditor.py	2010-04-30 11:35:43 +0000
 +++ bzrlib/msgeditor.py	2010-09-15 10:43:44 +0000
@@ -149,8 +149,10 @@
              return None
          edited_content = msg_transport.get_bytes(basename)
          if edited_content == reference_content:
--            if not ui.ui_factory.get_boolean(
--                "Commit message was not edited, use anyway"):
++            if not ui.ui_factory.confirm_action(
++                "Commit message was not edited, use anyway",
++                "bzrlib.msgeditor.unchanged",
++                {}):
                  # Returning "" makes cmd_commit raise 'empty commit message
                  # specified' which is a reasonable error, given the user has
                  # rejected using the unedited template.
 === modified file 'bzrlib/tests/blackbox/test_uncommit.py'
 --- bzrlib/tests/blackbox/test_uncommit.py	2010-03-01 16:18:43 +0000
 +++ bzrlib/tests/blackbox/test_uncommit.py	2010-09-15 10:43:44 +0000
@@ -22,7 +22,10 @@
  from bzrlib.bzrdir import BzrDirMetaFormat1
  from bzrlib.errors import BzrError, BoundBranchOutOfDate
  from bzrlib.tests import TestCaseWithTransport
--from bzrlib.tests.script import ScriptRunner
++from bzrlib.tests.script import (
++    run_script,
++    ScriptRunner,
++    )
  class TestUncommit(TestCaseWithTransport):
@@ -61,6 +64,20 @@
          out, err = self.run_bzr('status')
          self.assertEquals(out, 'modified:\n  a\n')
++    def test_uncommit_interactive(self):
++        """Uncommit seeks confirmation, and doesn't proceed without it."""
++        wt = self.create_simple_tree()
++        os.chdir('tree')
++        run_script(self, """
++        $ bzr uncommit
++        ...
++        The above revision(s) will be removed.
++        2>Uncommit these revisions? [y/n]:
++        <n
++        Canceled
++        """)
++        self.assertEqual(['a2'], wt.get_parent_ids())
++
      def test_uncommit_no_history(self):
          wt = self.make_branch_and_tree('tree')
          out, err = self.run_bzr('uncommit --force', retcode=1)
 === modified file 'bzrlib/tests/per_uifactory/__init__.py'
 --- bzrlib/tests/per_uifactory/__init__.py	2010-02-23 07:43:11 +0000
 +++ bzrlib/tests/per_uifactory/__init__.py	2010-09-15 10:43:44 +0000
@@ -62,6 +62,18 @@
          self.factory.be_quiet(False)
          self.assertEquals(False, self.factory.is_quiet())
++    def test_confirm_action(self):
++        # confirm_action should be answered by every ui factory; even
++        # noninteractive ones should have a reasonable default
++        self._load_responses([True])
++        result = self.factory.confirm_action(
++            'Break a lock?',
++            'bzr.lock.break.confirm',
++            {})
++        # will be true either because we read it from the input or because
++        # that's the default
++        self.assertEquals(result, True)
++
      def test_note(self):
          self.factory.note("a note to the user")
          self._check_note("a note to the user")
@@ -150,6 +162,11 @@
          # Without a TTY, we shouldn't display anything
          self.assertEqual('', self.stderr.getvalue())
++    def _load_responses(self, responses):
++        self.factory.stdin.seek(0)
++        self.factory.stdin.writelines([(r and "y\n" or "n\n") for r in responses])
++        self.factory.stdin.seek(0)
++
  class TestTTYTextUIFactory(TestTextUIFactory):
@@ -222,6 +239,9 @@
      def _check_log_transport_activity_display_no_bytes(self):
          pass
++    def _load_responses(self, responses):
++        pass
++
  class TestCannedInputUIFactory(tests.TestCase, UIFactoryTestMixin):
      # discards output, reads input from variables
@@ -250,3 +270,6 @@
      def _check_log_transport_activity_display_no_bytes(self):
          pass
++
++    def _load_responses(self, responses):
++        self.factory.responses.extend(responses)
 === modified file 'bzrlib/tests/test_ui.py'
 --- bzrlib/tests/test_ui.py	2010-09-14 08:14:22 +0000
 +++ bzrlib/tests/test_ui.py	2010-09-15 10:43:44 +0000
@@ -18,11 +18,12 @@
  """
  import os
--import re
  import time
  from StringIO import StringIO
++from testtools.matchers import *
++
  from bzrlib import (
      config,
      errors,
@@ -53,16 +54,28 @@
          ui = tests.TestUIFactory(stdin=None,
              stdout=tests.StringIOWrapper(),
              stderr=tests.StringIOWrapper())
--        os = ui.make_output_stream()
--        self.assertEquals(os.encoding, enc)
++        output = ui.make_output_stream()
++        self.assertEquals(output.encoding, enc)
  class TestTextUIFactory(tests.TestCase):
++    def make_test_ui_factory(self, stdin_contents):
++        ui = tests.TestUIFactory(stdin=stdin_contents,
++                                 stdout=tests.StringIOWrapper(),
++                                 stderr=tests.StringIOWrapper())
++        return ui
++
++    def test_text_factory_confirm(self):
++        # turns into reading a regular boolean
++        ui = self.make_test_ui_factory('n\n')
++        self.assertEquals(ui.confirm_action('Should %(thing)s pass?',
++            'bzrlib.tests.test_ui.confirmation',
++            {'thing': 'this'},),
++            False)
++
      def test_text_factory_ascii_password(self):
--        ui = tests.TestUIFactory(stdin='secret\n',
--                                 stdout=tests.StringIOWrapper(),
--                                 stderr=tests.StringIOWrapper())
++        ui = self.make_test_ui_factory('secret\n')
          pb = ui.nested_progress_bar()
          try:
              self.assertEqual('secret',
@@ -83,9 +96,7 @@
          We can't predict what encoding users will have for stdin, so we force
          it to utf8 to test that we transport the password correctly.
          """
--        ui = tests.TestUIFactory(stdin=u'baz\u1234'.encode('utf8'),
--                                 stdout=tests.StringIOWrapper(),
--                                 stderr=tests.StringIOWrapper())
++        ui = self.make_test_ui_factory(u'baz\u1234'.encode('utf8'))
          ui.stderr.encoding = ui.stdout.encoding = ui.stdin.encoding = 'utf8'
          pb = ui.nested_progress_bar()
          try:
@@ -437,6 +448,39 @@
          self.assertIsNone('off', av)
++class TestConfirmationUserInterfacePolicy(tests.TestCase):
++
++    def test_confirm_action_default(self):
++        base_ui = _mod_ui.NoninteractiveUIFactory()
++        for answer in [True, False]:
++            self.assertEquals(
++                _mod_ui.ConfirmationUserInterfacePolicy(base_ui, answer, {})
++                .confirm_action("Do something?",
++                    "bzrlib.tests.do_something", {}),
++                answer)
++
++    def test_confirm_action_specific(self):
++        base_ui = _mod_ui.NoninteractiveUIFactory()
++        for default_answer in [True, False]:
++            for specific_answer in [True, False]:
++                for conf_id in ['given_id', 'other_id']:
++                    wrapper = _mod_ui.ConfirmationUserInterfacePolicy(
++                        base_ui, default_answer, dict(given_id=specific_answer))
++                    result = wrapper.confirm_action("Do something?", conf_id, {})
++                    if conf_id == 'given_id':
++                        self.assertEquals(result, specific_answer)
++                    else:
++                        self.assertEquals(result, default_answer)
++
++    def test_repr(self):
++        base_ui = _mod_ui.NoninteractiveUIFactory()
++        wrapper = _mod_ui.ConfirmationUserInterfacePolicy(
++            base_ui, True, dict(a=2))
++        self.assertThat(repr(wrapper),
++            Equals("ConfirmationUserInterfacePolicy("
++                "NoninteractiveUIFactory(), True, {'a': 2})"))
++
++
  class TestProgressRecordingUI(tests.TestCase):
      """Test test-oriented UIFactory that records progress updates"""
 === modified file 'bzrlib/ui/__init__.py'
 --- bzrlib/ui/__init__.py	2010-06-25 20:34:05 +0000
 +++ bzrlib/ui/__init__.py	2010-09-15 10:43:44 +0000
@@ -100,6 +100,42 @@
      return val
++class ConfirmationUserInterfacePolicy(object):
++    """Wrapper for a UIFactory that allows or denies all confirmed actions."""
++
++    def __init__(self, wrapped_ui, default_answer, specific_answers):
++        """Generate a proxy UI that does no confirmations.
++
++        :param wrapped_ui: Underlying UIFactory.
++        :param default_answer: Bool for whether requests for
++            confirmation from the user should be noninteractively accepted or
++            denied.
++        :param specific_answers: Map from confirmation_id to bool answer.
++        """
++        self.wrapped_ui = wrapped_ui
++        self.default_answer = default_answer
++        self.specific_answers = specific_answers
++
++    def __getattr__(self, name):
++        return getattr(self.wrapped_ui, name)
++
++    def __repr__(self):
++        return '%s(%r, %r, %r)' % (
++            self.__class__.__name__,
++            self.wrapped_ui,
++            self.default_answer,
++            self.specific_answers)
++
++    def confirm_action(self, prompt, confirmation_id, prompt_kwargs):
++        if confirmation_id in self.specific_answers:
++            return self.specific_answers[confirmation_id]
++        elif self.default_answer is not None:
++            return self.default_answer
++        else:
++            return self.wrapped_ui.confirm_action(
++                prompt, confirmation_id, prompt_kwargs)
++
++
  class UIFactory(object):
      """UI abstraction.
@@ -151,6 +187,24 @@
          """
          self._quiet = state
++    def confirm_action(self, prompt, confirmation_id, prompt_kwargs):
++        """Seek user confirmation for an action.
++
++        If the UI is noninteractive, or the user does not want to be asked
++        about this action, True is returned, indicating bzr should just
++        proceed.
++
++        The confirmation id allows the user to configure certain actions to
++        always be confirmed or always denied, and for UIs to specialize the
++        display of particular confirmations.
++
++        :param prompt: Suggested text to display to the user.
++        :param prompt_kwargs: A dictionary of arguments that can be
++            string-interpolated into the prompt.
++        :param confirmation_id: Unique string identifier for the confirmation.
++        """
++        return self.get_boolean(prompt % prompt_kwargs)
++
      def get_password(self, prompt='', **kwargs):
          """Prompt the user for a password.
@@ -377,7 +431,17 @@
                  "without an upgrade path.\n" % (inter.target._format,))
--class SilentUIFactory(UIFactory):
++class NoninteractiveUIFactory(UIFactory):
++    """Base class for UIs with no user."""
++
++    def confirm_action(self, prompt, confirmation_id, prompt_kwargs):
++        return True
++
++    def __repr__(self):
++        return '%s()' % (self.__class__.__name__, )
++
++
++class SilentUIFactory(NoninteractiveUIFactory):
      """A UI Factory which never prints anything.
      This is the default UI, if another one is never registered by a program
@@ -418,6 +482,9 @@
      def __repr__(self):
          return "%s(%r)" % (self.__class__.__name__, self.responses)
++    def confirm_action(self, prompt, confirmation_id, args):
++        return self.get_boolean(prompt % args)
++
      def get_boolean(self, prompt):
          return self.responses.pop(0)
 === modified file 'doc/developers/ui.txt'
 --- doc/developers/ui.txt	2010-09-03 09:14:12 +0000
 +++ doc/developers/ui.txt	2010-09-15 10:43:44 +0000
@@ -1,6 +1,6 @@
--==========================================
--Bazaar Developer Guide to User Interaction
--==========================================
++=========================
++Interacting with the user
++=========================
  Getting Input
  =============
@@ -24,6 +24,40 @@
  used for programmer convenience, but not performing unpredictably in the
  presence of different locales.
++Confirmation
++============
++
++There are some operations, such as uncommitting, or breaking a lock, where
++bzr may want to get confirmation from the user before proceeding.
++However in some circumstances bzr should just go ahead without asking: if
++it's being used from a noninteractive program, or if the user's asked to
++never be asked for this particular confirmation or for any confirmations
++at all.
++
++We provide a special UIFactory method `confirm_action` to do this.  It
++takes a `confirmation_id` parameter that acts as a symbolic name for the
++type of confirmation, so the user can configure them off.  (This is not
++implemented at present.)  GUIs can have a "don't ask me again" option
++keyed by the confirmation id.
++
++Confirmation ids look like Python paths to the logical code that should
++use them.  (Because code may move or the check may for implementation
++reasons be done elsewhere, they need not perfectly correspond to the place
++they're used, and they should stay stable even when the code moves.)
++
++``bzrlib.builtins.uncommit``
++    Before the ``uncommit`` command actually changes the branch.
++
++``bzrlib.lockdir.break``
++    Before breaking a lock.
++
++``bzrlib.msgeditor.unchanged``
++    Proceed even though the user made no changes to the template message.
++
++Interactive confirmations can be overridden by using a
++`ConfirmationUserInterfacePolicy` decorator as the default
++ui_factory.
++
  Writing Output
  ==============