Some comments below… +o pedantic-python
> On Jan 5, 2026, at 3:48 AM, George V. Neville-Neil <[email protected]> wrote: > > The branch main has been updated by gnn: > > URL: > https://cgit.FreeBSD.org/src/commit/?id=eb1c0d74cbb99f329767b3d565ae57a3ec032bee > > commit eb1c0d74cbb99f329767b3d565ae57a3ec032bee > Author: George V. Neville-Neil <[email protected]> > AuthorDate: 2026-01-05 11:40:12 +0000 > Commit: George V. Neville-Neil <[email protected]> > CommitDate: 2026-01-05 11:40:12 +0000 > > Convert fully to Python 3. Remove licence text, only keep > > SPDX. > > Update to use argparse rather than OptionParser (now deprecated). All good stuff :). > -import sys > import subprocess > from subprocess import PIPE > +import argparse Please sort imports — thanks :). > -# Use input() for Python version 3 > -if sys.version_info[0] == 3: > - raw_input = input > +def gather_counters(): > + """Run program and return output as array of lines.""" > + result = subprocess.run("pmccontrol -L", shell=True, > capture_output=True, text=True) Instead of using `shell=True`, please use `[“pmccontrol”, “-L”] — this employs best practices and avoids complaints from bandit [1] and ruff [2]. Will this command ever return non-UTF8 encodable characters? If so, this call will crash when decoding the output. > + tabbed = result.stdout.strip().split('\n') Please use `.splitlines()` instead of `.split(“\n”)`: it avoids hardcoding the text form of the character and achieves the same thing as the bytes form. Plus, it’s a bit easier to read/grok for humans. > + return [line.replace('\t', '') for line in tabbed] > > # A list of strings that are not really counters, just > # name tags that are output by pmccontrol -L > @@ -62,37 +36,28 @@ notcounter = ["IAF", "IAP", "TSC", "UNC", "UCF", "UCP", > "SOFT" ] > > def main(): > > - from optparse import OptionParser > - > - parser = OptionParser() > - parser.add_option("-p", "--program", dest="program", > - help="program to execute") > - parser.add_option("-w", "--wait", action="store_true", dest="wait", > - default=True, help="wait after each execution") > + parser = argparse.ArgumentParser(description='Exercise a program under > hwpmc') > + parser.add_argument('--program', type=str, required=True, help='target > program') > + parser.add_argument('--wait', action='store_true', help='Wait after each > counter.') > > - (options, args) = parser.parse_args() > + args = parser.parse_args() > > - if (options.program == None): > - print("specify program, such as ls, with -p/--program") > - sys.exit() > - > - p = subprocess.Popen(["pmccontrol", "-L"], stdout=PIPE) > - counters = p.communicate()[0] > + counters = gather_counters() > > if len(counters) <= 0: This is tautologically impossible for < 0. PEP8 says to express it this way: `If not counters:` > print("no counters found") > sys.exit() > > - for counter in counters.split(): > + for counter in counters: > if counter in notcounter: > continue > - p = subprocess.Popen(["pmcstat", "-p", counter, options.program], > - stdout=PIPE) > - result = p.communicate()[0] > + p = subprocess.Popen(["pmcstat", "-p", counter, args.program], > + text=True, stderr=PIPE) > + result = p.communicate()[1] > print(result) > - if (options.wait == True): > + if (args.wait == True): `if args.wait:` is the equivalent simpler PEP8-compatible form. You don’t need to compare against True/False explicitly. > try: > - value = raw_input("next?") > + value = input("Waitin for you to press ENTER") This wording’s a bit more direct and fixes a typo: `"Please press ENTER”`. > except EOFError: > sys.exit() Cheers! -Enji 1. https://bandit.readthedocs.io/en/latest/ 2. https://docs.astral.sh/ruff/
