On Tue, 2019-02-05 at 17:53 +0000, Daniel P. Berrangé wrote:
[...]
> - flattened = []
> + pkgs = []
> for item in temp:
> - if temp[item] is not None and temp[item] not in flattened:
> - flattened += [temp[item]]
> + pkgname = temp[item]
> + if pkgname is None:
> + continue
> + if pkgname not in pkgs:
> + pkgs.append(pkgname)
Nothing against refactoring the code this way, but such a change
belongs in its own patch.
[...]
> - sys.stdout.write("ENV PACKAGES ")
> - sys.stdout.write(" \\\n ".join(sorted(flattened)))
> -
> + varmap = {}
> + varmap["pkgs"] = "".join([" \\\n " + pkgname
> + for pkgname in sorted(pkgs)])
Unlike the original, this pointlessly loops through sorted(pkgs)
one additional time and adds a phantom element to the beginning of
the list. So, once you put everything together...
> if package_format == "deb":
> sys.stdout.write(textwrap.dedent("""
> RUN DEBIAN_FRONTEND=noninteractive && \\
> ( \\
> apt-get update && \\
> apt-get dist-upgrade -y && \\
> - apt-get install --no-install-recommends -y ${PACKAGES} && \\
> + apt-get install --no-install-recommends -y %(pkgs)s && \\
> apt-get autoremove -y && \\
> apt-get autoclean -y \\
> )
... the result looks like
apt-get install --no-install-recommends -y \
augeas-tools \
autoconf \
automake \
...
Notice the double space between '-y' and '\' as well as the weird
indentation. A better solution would be to do
varmap["pkgs"] = " \\\n ".join(sorted(pkgs))
followed by
sys.stdout.write(textwrap.dedent("""
...
apt-get install --no-install-recommends -y \\
%(pkgs)s && \\
apt-get autoremove -y && \\
...
which is clearer and results in the more pleasant
apt-get install --no-install-recommends -y \
augeas-tools \
autoconf \
automake \
...
> - """))
> + """) % varmap )
Again, I don't have a problem with using this syntax for string
formatting (and here it's actually much clearer than the one we
are currently using), but I don't want the script to become
inconsistent so we have to stick to a single style.
--
Andrea Bolognani / Red Hat / Virtualization
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
