[PR PATCH] More py2 py3 compat

Douglas Bagnall douglas.bagnall at catalyst.net.nz
Sun May 6 11:51:40 UTC 2018


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

cheers,
Douglas



More information about the samba-technical mailing list