Date: Sat, 10 Jan 2026 21:25:52 -0800 From: "Enji Cooper (yaneurabeya)" <yaneurabeya@gmail.com> To: "George V. Neville-Neil" <gnn@FreeBSD.org> Cc: "src-committers@freebsd.org" <src-committers@FreeBSD.org>, "dev-commits-src-all@freebsd.org" <dev-commits-src-all@FreeBSD.org>, "dev-commits-src-main@freebsd.org" <dev-commits-src-main@FreeBSD.org> Subject: Re: git: eb1c0d74cbb9 - main - Convert fully to Python 3. Remove licence text, only keep Message-ID: <76722189-7324-4BAE-9359-65948F51DDD1@gmail.com> In-Reply-To: <695ba4fb.35e3f.3a916a95@gitrepo.freebsd.org>
index | next in thread | previous in thread | raw e-mail
[-- Attachment #1 --] 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/ [-- Attachment #2 --] <html><head><meta http-equiv="content-type" content="text/html; charset=utf-8"></head><body style="overflow-wrap: break-word; -webkit-nbsp-mode: space; line-break: after-white-space;">Some comments below…<div><br></div><div>+o pedantic-python<br id="lineBreakAtBeginningOfMessage"><div><br><blockquote type="cite"><div>On Jan 5, 2026, at 3:48 AM, George V. Neville-Neil <gnn@FreeBSD.org> wrote:</div><br class="Apple-interchange-newline"><div><div>The branch main has been updated by gnn:<br><br>URL: https://cgit.FreeBSD.org/src/commit/?id=eb1c0d74cbb99f329767b3d565ae57a3ec032bee<br><br>commit eb1c0d74cbb99f329767b3d565ae57a3ec032bee<br>Author: George V. Neville-Neil <gnn@FreeBSD.org><br>AuthorDate: 2026-01-05 11:40:12 +0000<br>Commit: George V. Neville-Neil <gnn@FreeBSD.org><br>CommitDate: 2026-01-05 11:40:12 +0000<br><br> Convert fully to Python 3. Remove licence text, only keep<br><br> SPDX.<br><br> Update to use argparse rather than OptionParser (now deprecated).<br></div></div></blockquote><div><br></div>All good stuff :).</div><div><br><blockquote type="cite"><div><div>-import sys<br> import subprocess<br> from subprocess import PIPE<br>+import argparse<br></div></div></blockquote><div><br></div>Please sort imports — thanks :).<br><br><blockquote type="cite"><div><div>-# Use input() for Python version 3<br>-if sys.version_info[0] == 3:<br>- raw_input = input<br>+def gather_counters():<br>+ """Run program and return output as array of lines."""<br>+ result = subprocess.run("pmccontrol -L", shell=True, capture_output=True, text=True)<br></div></div></blockquote><div><br></div>Instead of using `shell=True`, please use `[“pmccontrol”, “-L”] — this employs best practices and avoids complaints from bandit [1] and ruff [2].</div><div><br></div><div>Will this command ever return non-UTF8 encodable characters? If so, this call will crash when decoding the output.</div><div><br><blockquote type="cite"><div><div>+ tabbed = result.stdout.strip().split('\n')<br></div></div></blockquote><div><br></div>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.</div><div><br><blockquote type="cite"><div><div>+ return [line.replace('\t', '') for line in tabbed]<br><br> # A list of strings that are not really counters, just<br> # name tags that are output by pmccontrol -L<br>@@ -62,37 +36,28 @@ notcounter = ["IAF", "IAP", "TSC", "UNC", "UCF", "UCP", "SOFT" ]<br><br> def main():<br><br>- from optparse import OptionParser<br>- <br>- parser = OptionParser()<br>- parser.add_option("-p", "--program", dest="program", <br>- help="program to execute")<br>- parser.add_option("-w", "--wait", action="store_true", dest="wait",<br>- default=True, help="wait after each execution")<br>+ parser = argparse.ArgumentParser(description='Exercise a program under hwpmc')<br>+ parser.add_argument('--program', type=str, required=True, help='target program')<br>+ parser.add_argument('--wait', action='store_true', help='Wait after each counter.')<br><br>- (options, args) = parser.parse_args()<br>+ args = parser.parse_args()<br><br>- if (options.program == None):<br>- print("specify program, such as ls, with -p/--program")<br>- sys.exit()<br>- <br>- p = subprocess.Popen(["pmccontrol", "-L"], stdout=PIPE)<br>- counters = p.communicate()[0]<br>+ counters = gather_counters()<br><br> if len(counters) <= 0:<br></div></div></blockquote><div><br></div>This is tautologically impossible for < 0. PEP8 says to express it this way:</div><div><br></div><div>`If not counters:`</div><div><br><blockquote type="cite"><div><div> print("no counters found")<br> sys.exit()<br><br>- for counter in counters.split():<br>+ for counter in counters:<br> if counter in notcounter:<br> continue<br>- p = subprocess.Popen(["pmcstat", "-p", counter, options.program],<br>- stdout=PIPE)<br>- result = p.communicate()[0]<br>+ p = subprocess.Popen(["pmcstat", "-p", counter, args.program],<br>+ text=True, stderr=PIPE)<br>+ result = p.communicate()[1]<br> print(result)<br>- if (options.wait == True):<br>+ if (args.wait == True):<br></div></div></blockquote><div><br></div>`if args.wait:` is the equivalent simpler PEP8-compatible form. You don’t need to compare against True/False explicitly.</div><div><br></div><div><blockquote type="cite"><div><div> try:<br>- value = raw_input("next?")<br>+ value = input("Waitin for you to press ENTER")<br></div></div></blockquote><div><br></div><div>This wording’s a bit more direct and fixes a typo: `"Please press ENTER”`.</div><br><blockquote type="cite"><div><div> except EOFError:<br> sys.exit()<br></div></div></blockquote><br></div></div><div>Cheers!</div><div>-Enji</div><div><br></div><div>1. <a href="https://bandit.readthedocs.io/en/latest/">https://bandit.readthedocs.io/en/latest/</a></div><div>2. <a href="https://docs.astral.sh/ruff/">https://docs.astral.sh/ruff/</a></div></body></html>home | help
Want to link to this message? Use this
URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?76722189-7324-4BAE-9359-65948F51DDD1>