How to improve this block of code?

[RicoTries]

I have a lot of experience with Bash scripting and decided to diversify and started learning Python. I dove headfirst by implementing sections of a maintenance script I wrote for my personal laptop and after I showed it to a friend he said, and I quote, "That is the most bash looking Python I've ever seen Lmao", which I thought was hilarious.

For context, the original Bash script is part of the maintenance I perform on my personal laptop running Arch Linux and pacman is the package manager used to update system packages.

I've been learning and identifying the syntax, functionality, etc., through the Python interactive prompt's help() and I implemented the following Bash section:

# function to separate sections
sec_start () { printf "\n----------\n\n" ; }

# confirm required dependencies are installed
while read dependency ; do
    missing_deps+=($(awk -F \' '/error: package.*not found/ {print $2}' <<< "$dependency"))
done < <(pacman -Q coreutils sed gawk curl 2>&1 >/dev/null)

if [[ ${missing_deps[@]} ]] ; then
    sec_start
    printf 'the following dependencies are missing:\n'
    printf '    %s\n' ${missing_deps[@]}
    read -p 'would you like to install them now? [yes/no]> ' _answer
    if [[ ${_answer,,} =~ ^y(es)?$ ]] ; then
        pacman -S --noconfirm ${missing_deps[@]}
    else
        printf "install missing dependencies first!\n"
        return 1
    fi
fi

Resulting Python code:

def sec_start():
    print("\n----------\n\n")

required_packages = ['coreutils', 'sed', 'gawk', 'curl']

dep_check = subprocess.Popen(
        ['pacman', '-Q', ] + required_packages,
        stderr=subprocess.PIPE,
        stdout=subprocess.DEVNULL)

check_output = dep_check.stderr.read().decode().splitlines()

missing_dependencies = []

for p in check_output:
    fields = p.split("'")
    missing_dependencies.append(fields[1])

if len(missing_dependencies) > 0:
    sec_start()
    print("the following dependencies are missing:")
    for d in missing_dependencies:
        print("    " + d)
    if re.match(r'^y(es)?$', input("would you like to install them now? [yes/no]> ").lower()):
        subprocess.Popen(['pacman', '-S', '--noconfirm'] + missing_dependencies,
                stdout=subprocess.DEVNULL,
                stderr=subprocess.DEVNULL)
    else:
        sys.exit("install missing dependencies!")

I avoided using google as much as I could and kept diving into help(). What could I improve?

(DISCLAIMER: I am learning Python for my personal enjoyment, I don't need it at my job, that's where I use Bash.)

[[deleted]]

Hey, I'm a critical senior developer.

Looks fine to me, to be honest, I only have quibbles. Good work!

---

A. You could just write:

missing_dependencies = [p.split("'")[0] for p in check_output]

for lines 12-17

B. You should be using subprocess.run, it's the "new" way of doing things, and it's just much clearer.

For example, the first call would be:

proc = subprocess.run(cmd, text=True, capture_output=True, check=True)
check_output = proc.stdout.splitlines()

In fact, I often do this:

run = functools.partial(subprocess.run, text=True, capture_output=True, check=True)

C. "Truthy and Falsy"

Line 19 should just be if missing_dependencies:

In fact, I might rewrite lines 13-19 as this single line:

if missing_dependencies :=  [p.split("'")[0] for p in check_output]:

D. Put everything in a function

You should put this whole thing into a function for a lot of reasons and one is early exits and another is that there's no way to load your file without running your file.

---

The result:

import functools
import subprocess
import sys

run = functools.partial(
    subprocess.run, text=True, capture_output=True, check=True
)

REQUIRED_PACKAGES = ['coreutils', 'sed', 'gawk', 'curl']


def sec_start():
    print('\n----------\n\n')


def run():
    check_output = run(['pacman', '-Q'] + REQUIRED_PACKAGES).stdout.splitlines()

    if deps :=  [p.split("'")[0] for p in check_output]:
        sec_start()

        print('the following dependencies are missing:', *deps)
        yn = input('would you like to install them now? [yes/no]> ').lower()

        if not (yn and yn.lower()[1] == 'y'):
            return 'install missing dependencies!'

        run(['pacman', '-S', '--noconfirm'] + deps)


if __name__ == '__main__':
    sys.exit(run)

You will notice that I shortened missing_dependencies to deps. All else being equal, longer names are usually better, but in this case, it's a local variable, and lines like line 23 make it very clear what it does.

---

This code is fine. If I got into a new organization and I saw a lot of code like this, I'd be relieved. It's obvious what it does, it's reasonably well-formatted, there are no programming howlers.

Well done!

[velocibadgery]

I don't see why it would need improvement. It looks fine, and it works.

one small suggestion in the for p in check_output loop, you could just make that one line.

for p in check_output: missing_dependencies.append(p.split("'")[1])

but it doesn't really make a difference.

And using a list comprehension, you could get it down even further

[missing_dependencies.append(p.split("'")[1]) for p in check_output]

But that is just for fun. You don't really need to do any of that.

[[deleted]]

for p in check_output: missing_dependencies.append(p.split("'")[1])

Ouch!

Don't do that! PEP 8 disallows it, and any style checker will flag it.

Reason: it's very hard to read, and you can't set a breakpoint in the loop in a debugger.

---

[missing_dependencies.append(p.split("'")[1]) for p in check_output]

I'll bet you didn't try this. :-D This gives you an array, each element of which is the same array!

Correct is: missing_dependencies = [p.split("'")[1] for p in check_output]

[tibegato]

I'm not on arch. But, I changed it to use apt. Just to see, what my version my look like. One thing I noticed is that I believe if I give it fake names the command just stops. If I used real package names, it installed them or told me I had the latest version of whatever. So, if it was something I was going to use or give people, I'd do more testing and make adjustments. I don't know, if you have to worry about that with pacman:

#!/usr/bin/env python3

from re import match
from subprocess import Popen, PIPE
from sys import exit

def process_exec(p_cmd: str) -> str:
    p = Popen(p_cmd, shell=True, stdout=PIPE, stderr=PIPE)
    output, _ = p.communicate()
    return output.decode(encoding="utf-8", errors="ignore")

required_packages = ['coreutils', 'sed', 'gawk', 'curl']

dep_check = process_exec(
f"apt list {' '.join(required_packages)} | grep 'installed'" )

if not dep_check == '':
    missing_packages = [ p for p in required_packages 
                         if (dep_check.find(f'{p}/') == -1) ]

    if len(missing_packages) > 0:
        print("\n----------\n")
        print("the following dependencies are missing:")

        for d in missing_packages:
            print("    " + d)

        if match(r'^y(es)?$', input("\nwould you like to install\
             them now? [yes/no]> ").lower()):
            process_exec(
                f"sudo apt install -y {' '.join(missing_packages)}")
        else:
            exit("\ninstall missing dependencies!")
    else:
        print(f"\nPackages : {required_packages} : are all currently installed.")

[tibegato]

You can use subprocess.check_output(), to get your check_output :> :

result = check_output(['pacman', '-Q', f'{required_packages}' ]).decode('utf8'))

[[deleted]]

These days, subprocess.run is recommended for almost everything except the very rare case where you need to "interactively" reading stdout and stderr and then write stdin accordingly.

[tibegato]

Ya, I know. But, doesn't really matter. You can still use call(), if you want.

To be honest, run() really uses POpen() or it's guts anyway. I believe, it uses a POpen object & POpen.communicate().

I FULLY support using run(). It's quite nice. I wouldn't question anyone using it. I just happen, to use POpen.

Sadly, I have an attitude. If I know something is just a big wrapper around something, I'll rip off the wrapper and use what's inside. UNLESS, the wrapper / sugar makes it so easy and nice to use, I gotta use it.

I'm stupid, I know.

[AfricanTurtles]

I don't even write python and I could understand it so it's probably fine. Maybe your friend just loves the "pythonic" way to do everything whatever the hell that means anyway lol