[SCM] Samba Shared Repository - branch master updated
jelmer at samba.org
Thu Oct 13 11:27:26 MDT 2011
-----BEGIN PGP SIGNED MESSAGE-----
On 10/13/2011 07:06 PM, Giampaolo Lauria wrote:
> 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.
That's not the case - your changeset even included an exception to this,
Putting this sort of thing in the base class can be useful if it
abstracts everything away from the child classes, but that is not the
case. The subclasses now have to accept "magic" sambaopts, credopts and
> 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?
Commands that work locally don't take credentials options. "samba-tool
ntacl get" would be a good example.
Not all Samba commands should necessarily need a smb.conf file (although
all current ones seem to).
> 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.
I don't think there are any options, perhaps with the sole exception of
the version options, which are completely common to all commands.
Having a mix of options that are opt-in and options that are opt-out is
very confusing. I think it makes much more sense to just have everything
opt-in. Saving a couple of lines of code per command doesn't weigh up
against the loss in clarity of the code IMO.
> 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.
I don't think that would be a good idea. It would be inconsistent with
the rest of the Samba commands that aren't implemented in Python, which
have a separate group for the version options as well.
> 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
>> 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
> 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
> Can we please revert this?
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.11 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/
-----END PGP SIGNATURE-----
More information about the samba-technical