[SCM] Samba Shared Repository - branch master updated

[Jelmer Vernooij]

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

On 10/13/2011 09:46 PM, Giampaolo Lauria wrote:
>
> 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.
Sorry, I meant "samba-tool testparm". Your patch adds credentials
arguments to it for no good reason.
>
>
> 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.
Sure, but it's nonobvious where those arguments are coming from. And it
makes it way too easy for commands to take options they don't actually
need (your patch does this for "samba-tool testparm").

Lots of options

It's about introducing the least amount of surprise to whoever is
implementing a samba-tool subcommand, making it obvious why something is
happening. A lot of options take a server_name as argument. That doesn't
mean we should have takes_args default to ["server_name?"].
>
>
> > 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?
It might take those options, but it's not using them.
>
> 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?
>
I'm not necessarily opposed to renaming it, though I don't have a good
idea for another name. IIUC "Common" doesn't have to imply it's shared
by all commands though, just by most.
>
> > 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.
Yes, but I'm talking about the other Samba command-line tools, not just
samba-tool.

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/

iQIcBAEBAgAGBQJOl1BuAAoJEACAbyvXKaRX8lQP/jNAmeqOGPmlM8mVt6QDAayN
l5UZMT60s3Fb+6JOoLEVEMtFidYHQlUFkmArzstJWa1kzQKfdj9wGwjlO2KqEcnC
b5cnOG2CMx6PIf/bSyHzdwoGRkYtO+vUlCnvvkOw2V/iDAkUnA89+MoQP84soZWi
b2lUJ4rdfy4HmV/ABAQZLQ6L8PtSmVlXYNS0kBZD1kU/8iQh0lFhl97ZHOFc0HOG
dy+RyJFLhheGTMMZuzT79GFb2NrSUJK96oN3tPPga41wDUzGSpCeHku6IWaOiwdq
FwGbqjBUJeG/WN2QHKoD1E9HSKjUNUQjKyw/npu+dvqd15VzQ33HflyzucUAWm6c
+7Bvd/wSRfZh0xwwVJkNzuMkDenEH2XioBKCba3LNMKhGT9B81I5s9hUQVUdaqaV
U+bog7ivepY0Yf8+fLnCMOyb+YqjsMpYdbB5RVGNYR4eU4CujioK4OWseaxTHb8g
tLuG5K54pGmOvv/nxA291tDw2b5OwrwO69dSmUV8haxDvh+/j9pHRtnh9V5zofhR
NUdCwtHOFI8FISGm+D4wj7M66qhlfkKWS37rONhQ9nD3E2+SJ3Q//AFLx48Lbk+k
YiAR8Zdz3Y+3JmdeOq9ykL3fGOIp/s6fGT3vEvN/9TdrgK+DVa1pF8jtg4tZSX4J
SgRy+SI3GKDglmwF1Szl
=9kri
-----END PGP SIGNATURE-----