Re: [Fab-user] Patch for prompt and require

[Christian Vest Hansen]

On Tue, Oct 21, 2008 at 3:15 PM, Niklas Lindström <address@hidden> wrote:
> Hi again!
>
> I have pushed a new version of my branch that I hope is good enough
> for review. I have a couple of things in there which I'll describe
> below.

Ok. I'll take a look at it if i can find the time later today.

>
> Firstly, I wonder if you feel comfortable diffing all these changes at
> once and cherry-pick? Or if you'd prefer that I did one thing at a
> time and passed it to you for review.

I prefer when each commit only concern one thing, be it feature, bug
fix, spelling or what ever. But people often don't work like that - I
know I don't - but that's where interactive adding (git add -i) can
really help, so even if you have a bunch of unrelated changes, you're
still able to bundle cohesive changes together.

It's nice when things are seperated, like when a range of consequtive
commits only concern one feature and go from start to finish, but I
don't work like that and i don't require other people to work like
that. However, I think rebasing can be used to simulate it to some
extent. I'm not totally sure, though. I have not really used rebasing
much.

>
> My desire is of course to contribute it all if you'll see it fit, but
> since I want to use the real Fabric and not my fork (we'll be a bunch
> of developers using the tool), I'd rather get off my fork and onto
> your master (and upcoming new release ;) ) soon enough.
>
>
> On Mon, Oct 20, 2008 at 6:26 PM, Christian Vest Hansen
> <address@hidden> wrote:
>
>>> Here's what I've continued with:
>>>
>>> On Sun, Oct 19, 2008 at 4:18 AM, Christian Vest Hansen
>>> <address@hidden> wrote:
>>> I understand your concerns with what I did with prompt(). I'm after
>>> three things:
>>>
>>> 1. To (always) repeat the prompt if validate fails. I skipped the
>>> 'repeat' flag in my new version.
>>>
>>> 2. To invoke a given validate even if we have a supplied value. I just
>>> removed my 'auto' keyword and changed the use of a present varname in
>>> ENV so that this gets validated as well..
>>>
>>> 3. I'd like to use regexp validations, so I've kept the overloaded
>>> 'validate' (and updated the docstring) which uses the RegexpValidator
>>> (previously named Matcher). Do you think allowing regexp strings is a
>>> bad idea? Perhaps we could keep RegexpValidator or similar, so at
>>> least that one is available as a convenience tool? Though think I'd
>>> prefer a less "obtrusive" helper..
>>
>> These three points make It _sound_ better.
>>
>> I don't know if Jeff has put any code towards any of his ideas
>> regarding prompt, though I don't see anything in the commit log for
>> his master branch.
>
> Nice. But do you think my version of `prompt` is good enough, or
> should I/we work with Jeff as well to arrive at something like this?

I still haven't looked at the code, so I don't know. It sounds like
you have it implemented already. If that is the case then I might as
well take a look at it and merge it if I like it. Then Jeff can
improve on the merged version if he wants to.

>
>
>>>>> * improved "load":
>>>>>    - will only invoke the loaded fab script once. Thus multi-file based 
>>>>> fab
>>>>>      setups can shared loaded scipts without having them invoked multiple
>>>>>      times.
>
> In addition to my last changes, `load` does now use execfile with a
> restricted namespace (built from the registries - plus `var`, see
> below). This way you can control what is accessible to the fabfiles,
> and also pick from the result (using the `captured` dict).

Try cd'ing to the prototype directory and run `fab test_imports
test_global_assignment` -- if it prints "all double-good" at the
bottom, then I'm happy. :)

I'm pretty sure that fabric did scope-limitation like that at some
point, but I _think_ I dropped it because it was the easiest way to
make imports and global assignments work across commands.

>
>
>>>>> * new operation "call_once":
>>>>>    - will call the given function (command) *unless* it has already been
>>>>>      called.
>>>>
>>>> These ideas I like. Especially the improvements to load.
>>>
>>> I have *preliminary* working versions of these. Especially call_once
>>> needs more thinking though (regarding messages, and if we could reuse
>>> the '_execute_commands' logic for parameters etc -- see my TODO
>>> scribble).
>>
>> You can use pull-request (github feature) or send an email when you
>> think it's ready.
>
> I have completed this "to my own taste", but I'd like for your review.
> `call_once`is now named `invoke`. The actual control of whether a
> command has been executed is put in your _`execute_commands`
> (actually, in the `_execute_command` I factored out). I did this since
> I believe it makes sense to not invoke the same command twice if you
> supply it on the command line and also reference it in `invoke` (or
> @depends).
>
> This should only change the current behaviour of Fabric in one way: if
> you give the command name twice to "fab", it will only run once. If
> you actually *want* to invoke a command twice or more, they can of
> course still be called as regular functions from other commands.

I'm not fond of the sound of that. For instance, there's a command
called `set` which has the same effect as `set` the operation.
Basically it allows you to set variables from the command line. I
think only @depends or `invoke` should have the if-not-already-done
logic.

>
>
>>>> These would be the first decorators intended for use in fabfiles.
>>>> You/we will have to think about how they are going to interoperate
>>>> with the list and help commands.
>>>
>>> Absolutely. I've added these, without any documentation, and will
>>> continue with how to register them and integrating that in the help
>>> system. '@depends' will also be aligned with how I'll proceed with
>>> improving 'call_once'.
>>
>> Sounds good :)
>
> I have added decorators to the registry and help system now. (But see
> my previous mail about the issues with `func_code`).
>
>
>>>>> == After that ==
>>>>>
>>> Basically I just like to (as Jeff also seemed to) solve the issue that
>>> 'set' currently shadows the builtin with the same name. We then toyed
>>> with the idea of exposing one object to replace the get/set (which I
>>> fully understand if you don't want to remove though for e.g.
>>> compatability, clean names etc). Since 'vars' is also a builtin, I
>>> thought of 'params'. Perhaps e.g. 'var' would be better.. Working
>>> something like:
>>>
>>>    # Define one or more variables (just as with current "set"):
>>>    var(fab_user='me')
>>>    var(fab_hosts=[...], fab_user='me')
>>>
>>>    # Define one var can also be done like:
>>>    var.fab_user = 'me'
>>>    var['fab_user'] = 'me'
>>>
>>>    # For getting them, same pattern:
>>>    print var.fab_user
>>>    print var['fab_user']
>>
>> Looks nice. I think `var` is a good name. I wasn't totally happy with
>> the shadowing when I introduced `set` but I couldn't come up with a
>> better name.
>
> I've implemented this as well, by turning `ENV` into an instance of
> `Variables`, which is a subclass of dict. (And prepopulated with
> `DEFAULT_ENV`.) It is passed to the fabfiles under the name `var` via
> execfile, as I described above.
>
>
> Again, I hope you'll find at least parts good enough to merge -- and
> if not, please instruct me with any fixes you'd desire.
>
> Best regards,
> Niklas
>



-- 
Venlig hilsen / Kind regards,
Christian Vest Hansen.