[SCM] Samba Shared Repository - branch master updated

[Giampaolo Lauria]

Hi Jelmer,

At the time I made the change, I was under the impression that the option 
groups were the same across all samba-tool commands. With that in mind, I 
thought that it would be more appropriate to define the option groups 
within the base class, not the derived ones.

I said it is a more object oriented approach because, in general, one 
tries to put all the common properties of an object in the base class.

In the optiongroups, we have Samba Common Options. Aren't these in common 
across all commands? What about Credential Options, which commands do not 
take these? 

Before reverting the change, I think we should think about what options 
are common across all commands and what are not. For those that are 
common, I think we should define them in the base class.

One more thing I'd like to propose, can we remove the Version Options 
group and put --version into the Samba Common Options? I don't see the 
reason for having a different group for that option.

Thanks,
Giampaolo




From:   Jelmer Vernooij <jelmer at samba.org>
To:     samba-technical at lists.samba.org
Cc:     Giampaolo Lauria <lauria2 at yahoo.com>
Date:   10/12/2011 07:04 PM
Subject:        Re: [SCM] Samba Shared Repository - branch master updated
Sent by:        samba-technical-bounces at lists.samba.org



On Thu, Jul 21, 2011 at 04:59:03AM +0200, Andrew Tridgell wrote:
> commit f6fa8684896b8f3f9f8b7bd3742c99906973274c
> Author: Giampaolo Lauria <lauria2 at yahoo.com>
> Date:   Fri Jul 15 12:07:03 2011 -0400

>     samba-tool: moved takes_optiongroups definition to Command base 
class

>     The option groups should be defined at the Command base class level 
as they are in common across all samba-tool commands.
>     Major move advantages:
>     1. more OOP approach
How is this more object oriented?

>     2. enforcing consistency across commands
>     3. avoiding the need of declaring for every new command
This is really confusing. It means that if you create a new command
you have to know about these default option groups, and the fact that
your run() method has to take 3 extra parameters. If you later
override takes_optiongroups some of these arguments will no longer be
specified.

It makes it harder to add a simple command, and gradually extend it.

Also, not every command takes these option groups, that's why they're
optional.

Can we please revert this?

Cheers,

Jelmer