[PR PATCH] More py2 py3 compat

[Noel Power]

On 06/05/18 12:51, Douglas Bagnall via samba-technical wrote:
> On 06/05/18 22:05, Github bot (Noel Power) wrote:
>> * xrange -> range Note: there is one instance of this where it might
>>   make a difference (see commit message) due to the potential amount
>>   of items in the range. Advice needed here, might need to drop one
>>   of these patches at the moment. Six apparently has a wrapper we
>>   could use instead (but CI doesn't support yet... I think)
> Which I assume is referring to this one:
>
>> From 6d1a2b54eab7897b1ff71dc84ff6c16df2d29c3e Mon Sep 17 00:00:00 2001
>> From: Noel Power <noel.power at suse.com>
>> Date: Fri, 4 May 2018 12:20:36 +0100
>> Subject: [PATCH 10/17] samba_tool: replace xrange -> range
yes :-)
>>
>> This one might be a better candidate for using six.moves as there
>> are perhaps issues with the amount of elements involved
>> Signed-off-by: Noel Power <noel.power at suse.com>
>> ---
>>  python/samba/netcmd/domain.py | 32 ++++++++++++++++----------------
>>  1 file changed, 16 insertions(+), 16 deletions(-)
> I wouldn't worry too much because samba-tool is slow anyway, but...
>
>> diff --git a/python/samba/netcmd/domain.py b/python/samba/netcmd/domain.py
>> index 1e242dea62e..5c92bb7f393 100644
>> --- a/python/samba/netcmd/domain.py
>> +++ b/python/samba/netcmd/domain.py
>> @@ -2011,7 +2011,7 @@ def write_forest_trust_info(self, fti, tln=None, collisions=None):
>>          self.outf.write("Namespaces[%d]%s:\n" % (
>>                          len(fti.entries), tln_string))
>>
>> -        for i in xrange(0, len(fti.entries)):
>> +        for i in range(0, len(fti.entries)):
>>              e = fti.entries[i]
> This would be better as
>
> -        for i in xrange(0, len(fti.entries)):
> -            e = fti.entries[i]
> +        for i, e in enumerate(fti.entries):
>
> or you could go further. To take the next example:
>
>>              flags = e.flags
>> @@ -3403,7 +3403,7 @@ def run(self, domain=None, sambaopts=None, localdcopts=None, versionopts=None,
>>
>>              for upn in add_upn:
>>                  idx = None
>> -                for i in xrange(0, len(update_upn_vals)):
>> +                for i in range(0, len(update_upn_vals)):
>>                      v = update_upn_vals[i]
>>                      if v.lower() != upn.lower():
>>                          continue
> @@ -3401,15 +3400,11 @@ class cmd_domain_trust_namespaces(DomainTrustCommand):
>              update_spn_vals.extend(stored_spn_vals)
>
>              for upn in add_upn:
> -                idx = None
> -                for i in xrange(0, len(update_upn_vals)):
> -                    v = update_upn_vals[i]
> -                    if v.lower() != upn.lower():
> -                        continue
> -                    idx = i
> -                    break
> -                if idx is not None:
> -                    raise CommandError("Entry already present for value[%s] specified for --add-upn-suffix" % upn)
> +                for v in update_upn_vals:
> +                    if v.lower() == upn.lower():
> +                        raise CommandError("Entry already present for "
> +                                           "value[%s] specified for "
> +                                           "--add-upn-suffix" % upn)
>                  update_upn_vals.append(upn)
>                  replace_upn = True
>
> If efficiency really mattered, it would be better to replace the
> update_upn_vals with something like an ordereddict where the keys were
> the lowercase upns. Then it would become:
>
> +                if upn.lower() in update_upn_vals_lower:
> +                    raise CommandError(...)
> +                update_upn_vals_lower[upn.lower()] = upn
>                  replace_upn = True
>
>
> but going to enumerate() is already an improvement in 2 and 3 and
> should be simple enough.
Thanks alot Douglas,
I'll take a closer look (only just catching up on mail from a long
weekend) and I'll fold this these changes and will resubmit

Noel