[PATCH] Allow editing groups from samba-tool

William Brown william at blackhats.net.au
Mon Apr 30 22:03:56 UTC 2018


> > > I know this was partially copied from the existing code but I'd
> > > rather
> > > pull the editor checks out of the loop and do this first thing in
> > > the
> > > +run_edit() so that we don't even try opening SamDB and search
> > > there
> > > for an LDAP record if editor is missing. Original code didn't do 
> > > 'raise CommandError' because it assumed 'vi' always exists on the
> > > system but if you are adding a terminal path here, better to move
> > > it
> > > outside of the loop.
> > 
> > Then why didn't you raise this in the first place ? I would have
> > fixed
> > it then.
> 
> The original code did not raise an exception, so it was fine as it
> backed to an asumingly always existing 'vi'. If we add a hard
> exception
> if an editor does not exist, we'd rather move the code out of a loop
> as the code is testing a fact that doesn't change within a loop and
> can
> avoid even reading the data in the first place.
> 
> BTW, 'vi' would not pass os.path.exists() check, so if EDITOR is not
> set, then the proposed patch would always raise an exception.
> Original
> code relied on a system()'s use of a shell PATH to search the 'vi' or
> any other non-fully specified editor binary.

This is also a good point. As mentioned I'll tidy this up today and
resubmit. 

> 
> > > > -Example1 shows how to edit a users attributes in the domain
> > > > against a remote +Example1 shows how to edit a user's
> > > > attributes in
> > > > the domain against a remote LDAP server.
> > 
> > Please don't add even more greengrocers apostrophes to Samba, it is
> > <users> not <users's>
> 
> It is a single user's record to be edited, not multiple users at the
> same time. And attributes belong to that record (so to the user, thus
> "user's"). I know, I'm venturing into a territory a non-native
> speaker
> would probably need to avoid but that's how I understand the help
> above
> and that's how the command is operating. To me using "users" would be
> totally against the meaning of the operation here.

Your knowledge of apostrophes is correct here Alex. This is why I made
the subtle change to "user's" because the attributes belong to the
user, we don't have multiple users to modify.

It's important that we take pride in all parts of our work from the
grammar of our documentation to the whitespacing and formatting of our
code patches. We have code review, so we should have "English review"
also. :) 

Thanks,

William




More information about the samba-technical mailing list