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

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...

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

>>>>> 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...

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.

> > 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.

-----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-----

>>>>> 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).

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

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'.

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

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.

Great !

sent to pqm by email

sent to pqm by email

sent to pqm by email

sent to pqm by email