[SCM] Samba Shared Repository - branch master updated

[Giampaolo Lauria]

From:   Jelmer Vernooij <jelmer at samba.org>
To:     samba-technical at lists.samba.org
Date:   10/13/2011 01:29 PM
Subject:        Re: [SCM] Samba Shared Repository - branch master updated
Sent by:        samba-technical-bounces at lists.samba.org




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

Hi Giampaolo,

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,
"samba-tool time".

I am not sure which exception you are referring to.


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

Overriding is a way for a subclass to not have to accept that.
>
>
> 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.

Before I made this change, "ntacl get" was using those options. Did we 
have a bug?


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.

If so, then should we rename Samba Common Options?

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.

All samba-tool commands have been implemented in Python for the last few 
months.

Cheers,

Jelmer

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

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

iQIcBAEBAgAGBQJOlx9+AAoJEACAbyvXKaRXgGcP/RSLehvTjN1MmylVg0CBm9MS
fKQufHveRm1oj0BdQHdzVWhxqSu36orGlpKmZU97gxlaBhVPVlA+xQTtvSu4DkdZ
YWmvgLPguwhLQB26DNvqtSf7UVdi1rnSrYXcWGifRhgN6DcVJVd26/V5BWzEDif3
XRKpzPS6SRs4naBjYULncv+fC2o3aJUzn/i3ZmlciPcSm+1SyUg0hkIwk4+IeQhR
2zrjk6s8/v7iFG+lJOPwOT5RbJ0YEZ/tHNGAGlQnIBWOF1kJeXz3Had4ldodtzds
pekoIdJBXS2SRd+k+f3CQkz8xtqrs7hp0jf0Hdb+eORsJAU92pCDf08zRFnyYfC+
UJqx4LS9ubafFeLO75nNt4DeGbYUrtIcDjghkgM6XzaqMAygEsPe2qfVAnfaPVtE
YfmpNVw8Bjw4QLepffuLvYNW8Sd2MJUAhq8iO81dYTco73kVWW3JUigrTi5VogRC
VDwxF/xGAfSGyiWREBDfUkxuhMeZRt1Nfwfg8fCyrrRF4pgdkcSnq16VGW+yJ27x
ESWrkstgYUPYKOcOvCpcx75+O05mnRAEmq07vIjEySt5ZI5YDeLXOqKw9WZJ96Kl
v6Cqglc/eEoW3jouMLQKyegdtFp1rjrIBwL4ZAuSr2EWTsux9ljZbwi+Ddu8Ig2d
LK1R0QAQ1oDyttmjH0W1
=zceO
-----END PGP SIGNATURE-----