Re: git: eb1c0d74cbb9 - main - Convert fully to Python 3. Remove licence text, only keep
Date: Sun, 11 Jan 2026 05:25:52 UTC
Some comments below…
+o pedantic-python
> On Jan 5, 2026, at 3:48 AM, George V. Neville-Neil <gnn@FreeBSD.org> 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 <gnn@FreeBSD.org>
> AuthorDate: 2026-01-05 11:40:12 +0000
> Commit: George V. Neville-Neil <gnn@FreeBSD.org>
> 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/