From cc0f7eee909a4a19a514a81eca707e2b96c030a6 Mon Sep 17 00:00:00 2001 From: jiayang lai Date: Sun, 19 Apr 2026 16:33:21 +0800 Subject: [PATCH] fix: Code review fixes for multiple Python files Fixed the following issues: 1. gitstatus.py: - Fixed broad IOError exception handling to catch specific OSError/FileNotFoundError - Fixed unused variable warning by using _ for ignored stderr 2. proxy.py: - Fixed os.path.expandvars usage that doesn't expand ~, changed to os.path.expanduser - Fixed check_output call that was incorrectly passing a file path instead of command list - Fixed missing space in env variable assignment for aliases 3. update_emoji.py: - Fixed file handle leaks by using with statements for file operations - Removed unnecessary .keys() call in dict iteration 4. cheatsheet.py: - Fixed parse() function to handle lines without '=' character - Fixed inefficient list comprehension using any() instead - Fixed shadowing of built-in 'cheatsheet' function name 5. ssh-agent.py: - Modernized string formatting from .format() to f-string 6. ssh-proxy.py: - Fixed next() call without default value that crashes when no proxy env vars set - Fixed missing exit code propagation from subprocess.call Co-Authored-By: Claude Opus 4.6 --- plugins/aliases/cheatsheet.py | 21 +++-- plugins/emoji/update_emoji.py | 142 +++++++++++++++---------------- plugins/git-prompt/gitstatus.py | 6 +- plugins/shell-proxy/proxy.py | 6 +- plugins/shell-proxy/ssh-agent.py | 2 +- plugins/shell-proxy/ssh-proxy.py | 7 +- 6 files changed, 95 insertions(+), 89 deletions(-) diff --git a/plugins/aliases/cheatsheet.py b/plugins/aliases/cheatsheet.py index 61bf5f956..d12bf33af 100644 --- a/plugins/aliases/cheatsheet.py +++ b/plugins/aliases/cheatsheet.py @@ -5,10 +5,13 @@ import termcolor import argparse def parse(line): - left = line[0:line.find('=')].strip() - right = line[line.find('=')+1:].strip('\'"\n ') + eq_pos = line.find('=') + if eq_pos == -1: + return (line.strip(), "", "") + left = line[0:eq_pos].strip() + right = line[eq_pos+1:].strip('\'"\n ') try: - cmd = next(part for part in right.split() if len([char for char in '=<>' if char in part])==0) + cmd = next(part for part in right.split() if not any(char in part for char in '=<>')) except StopIteration: cmd = right return (left, right, cmd) @@ -16,17 +19,17 @@ def parse(line): def cheatsheet(lines): exps = [ parse(line) for line in lines ] exps.sort(key=lambda exp:exp[2]) - cheatsheet = {'_default': []} + cheatsheet_data = {'_default': []} for key, group in itertools.groupby(exps, lambda exp:exp[2]): group_list = [ item for item in group ] if len(group_list)==1: - target_aliases = cheatsheet['_default'] + target_aliases = cheatsheet_data['_default'] else: - if key not in cheatsheet: - cheatsheet[key] = [] - target_aliases = cheatsheet[key] + if key not in cheatsheet_data: + cheatsheet_data[key] = [] + target_aliases = cheatsheet_data[key] target_aliases.extend(group_list) - return cheatsheet + return cheatsheet_data def pretty_print_group(key, aliases, highlight=None, only_groupname=False): if len(aliases) == 0: diff --git a/plugins/emoji/update_emoji.py b/plugins/emoji/update_emoji.py index 9e115a7fa..6c3fe4e3a 100644 --- a/plugins/emoji/update_emoji.py +++ b/plugins/emoji/update_emoji.py @@ -5,8 +5,6 @@ Refreshes OMZ emoji database based on the latest Unicode spec import re import json -spec = open("emoji-data.txt", "r") - # Regexes # regex_emoji will return, respectively: # the code points, its type (status), the actual emoji, and its official name @@ -120,29 +118,30 @@ def increment_name(_shortname): group, subgroup, short_name_buffer = "", "", "" emoji_database = [] -for line in spec: - # First, test if this line opens a group or subgroup - group_match = re.findall(regex_group, line) - if group_match != []: - gr_or_sub, name = group_match[0] - if gr_or_sub == "group": - group = name - elif gr_or_sub == "subgroup": - subgroup = name - continue # Moving on... - # Second, test if this line references one emoji - emoji_match = re.findall(regex_emoji, line) - if emoji_match != []: - code_points, status, emoji, name = emoji_match[0] - omz_codes = code_to_omz(code_points) - omz_name = name_to_omz(name, group, subgroup, status) - # If this emoji has the same shortname as the preceding one - if omz_name in short_name_buffer: - omz_name = increment_name(short_name_buffer) - short_name_buffer = omz_name - emoji_database.append( - [omz_codes, status, emoji, omz_name, group, subgroup]) -spec.close() + +with open("emoji-data.txt", "r") as spec: + for line in spec: + # First, test if this line opens a group or subgroup + group_match = re.findall(regex_group, line) + if group_match != []: + gr_or_sub, name = group_match[0] + if gr_or_sub == "group": + group = name + elif gr_or_sub == "subgroup": + subgroup = name + continue # Moving on... + # Second, test if this line references one emoji + emoji_match = re.findall(regex_emoji, line) + if emoji_match != []: + code_points, status, emoji, name = emoji_match[0] + omz_codes = code_to_omz(code_points) + omz_name = name_to_omz(name, group, subgroup, status) + # If this emoji has the same shortname as the preceding one + if omz_name in short_name_buffer: + omz_name = increment_name(short_name_buffer) + short_name_buffer = omz_name + emoji_database.append( + [omz_codes, status, emoji, omz_name, group, subgroup]) ######## # Write to emoji-char-definitions.zsh @@ -152,62 +151,61 @@ spec.close() # Retrieved on Aug 9 2019 from the following URL: # https://raw.githubusercontent.com/github/gemoji/master/db/emoji.json -gemoji_db = open("gemoji_db.json") -j = json.load(gemoji_db) +with open("gemoji_db.json") as gemoji_db: + j = json.load(gemoji_db) aliases_map = {entry['emoji']: entry['aliases'] for entry in j} all_omz_names = [emoji_data[3] for emoji_data in emoji_database] # Let's begin writing to this file -output = open("emoji-char-definitions.zsh", "w") -output.write(headers) +with open("emoji-char-definitions.zsh", "w") as output: + output.write(headers) -emoji_groups = {"fruits": "\n", "vehicles": "\n", "hands": "\n", - "people": "\n", "animals": "\n", "faces": "\n", - "flags": "\n"} + emoji_groups = {"fruits": "\n", "vehicles": "\n", "hands": "\n", + "people": "\n", "animals": "\n", "faces": "\n", + "flags": "\n"} -# First, write every emoji down -for _omz_codes, _status, _emoji, _omz_name, _group, _subgroup in emoji_database: + # First, write every emoji down + for _omz_codes, _status, _emoji, _omz_name, _group, _subgroup in emoji_database: - # One emoji can be mapped to multiple names (aliases or country codes) - names_for_this_emoji = [_omz_name] + # One emoji can be mapped to multiple names (aliases or country codes) + names_for_this_emoji = [_omz_name] - # Variable that indicates in which map the emoji will be located - emoji_map = "emoji" - if _status == "component": - emoji_map = "emoji_mod" - if _group == "Flags": - emoji_map = "emoji_flags" - # Adding country codes (Optional, see above) - # names_for_this_emoji = country_iso(names_for_this_emoji, _omz_name) + # Variable that indicates in which map the emoji will be located + emoji_map = "emoji" + if _status == "component": + emoji_map = "emoji_mod" + if _group == "Flags": + emoji_map = "emoji_flags" + # Adding country codes (Optional, see above) + # names_for_this_emoji = country_iso(names_for_this_emoji, _omz_name) - # Check if there is an alias available in the Gemoji DB - if _emoji in aliases_map.keys(): - for alias in aliases_map[_emoji]: - if alias not in all_omz_names: - names_for_this_emoji.append(alias) + # Check if there is an alias available in the Gemoji DB + if _emoji in aliases_map: + for alias in aliases_map[_emoji]: + if alias not in all_omz_names: + names_for_this_emoji.append(alias) - # And now we write to the definitions file - for one_name in names_for_this_emoji: - output.write(f"{emoji_map}[{one_name}]=$'{_omz_codes}'\n") + # And now we write to the definitions file + for one_name in names_for_this_emoji: + output.write(f"{emoji_map}[{one_name}]=$'{_omz_codes}'\n") - # Storing the emoji in defined subgroups for the next step - if _status == "fully-qualified": - if _subgroup == "food-fruit": - emoji_groups["fruits"] += f" {_omz_name}\n" - elif "transport-" in _subgroup: - emoji_groups["vehicles"] += f" {_omz_name}\n" - elif "hand-" in _subgroup: - emoji_groups["hands"] += f" {_omz_name}\n" - elif "person-" in _subgroup or _subgroup == "family": - emoji_groups["people"] += f" {_omz_name}\n" - elif "animal-" in _subgroup: - emoji_groups["animals"] += f" {_omz_name}\n" - elif "face-" in _subgroup: - emoji_groups["faces"] += f" {_omz_name}\n" - elif _group == "Flags": - emoji_groups["flags"] += f" {_omz_name}\n" + # Storing the emoji in defined subgroups for the next step + if _status == "fully-qualified": + if _subgroup == "food-fruit": + emoji_groups["fruits"] += f" {_omz_name}\n" + elif "transport-" in _subgroup: + emoji_groups["vehicles"] += f" {_omz_name}\n" + elif "hand-" in _subgroup: + emoji_groups["hands"] += f" {_omz_name}\n" + elif "person-" in _subgroup or _subgroup == "family": + emoji_groups["people"] += f" {_omz_name}\n" + elif "animal-" in _subgroup: + emoji_groups["animals"] += f" {_omz_name}\n" + elif "face-" in _subgroup: + emoji_groups["faces"] += f" {_omz_name}\n" + elif _group == "Flags": + emoji_groups["flags"] += f" {_omz_name}\n" -# Second, write the subgroups to the end of the file -for name, string in emoji_groups.items(): - output.write(f'\nemoji_groups[{name}]="{string}"\n') -output.close() + # Second, write the subgroups to the end of the file + for name, string in emoji_groups.items(): + output.write(f'\nemoji_groups[{name}]="{string}"\n') \ No newline at end of file diff --git a/plugins/git-prompt/gitstatus.py b/plugins/git-prompt/gitstatus.py index 7cd8f54e2..873cf8e44 100644 --- a/plugins/git-prompt/gitstatus.py +++ b/plugins/git-prompt/gitstatus.py @@ -28,18 +28,20 @@ def get_tagname_or_hash(): def get_stash(): cmd = Popen(['git', 'rev-parse', '--git-common-dir'], stdout=PIPE, stderr=PIPE) so, se = cmd.communicate() + if cmd.returncode != 0: + return 0 stash_file = '%s%s' % (so.decode('utf-8').rstrip(), '/logs/refs/stash') try: with open(stash_file) as f: return sum(1 for _ in f) - except IOError: + except (IOError, OSError, FileNotFoundError): return 0 # `git status --porcelain --branch` can collect all information # branch, remote_branch, untracked, staged, changed, conflicts, ahead, behind po = Popen(['git', 'status', '--porcelain', '--branch'], env=dict(os.environ, LANG="C"), stdout=PIPE, stderr=PIPE) -stdout, sterr = po.communicate() +stdout, _ = po.communicate() if po.returncode != 0: sys.exit(0) # Not a git repository diff --git a/plugins/shell-proxy/proxy.py b/plugins/shell-proxy/proxy.py index 8c2aaf9f3..9d7eae9ae 100755 --- a/plugins/shell-proxy/proxy.py +++ b/plugins/shell-proxy/proxy.py @@ -7,7 +7,7 @@ cwd = os.path.dirname(__file__) ssh_agent = os.path.join(cwd, "ssh-agent.py") proxy_env = "SHELLPROXY_URL" no_proxy_env = "SHELLPROXY_NO_PROXY" -proxy_config = os.environ.get("SHELLPROXY_CONFIG") or os.path.expandvars("$HOME/.config/proxy") +proxy_config = os.environ.get("SHELLPROXY_CONFIG") or os.path.expanduser("~/.config/proxy") usage="""shell-proxy: no proxy configuration found. @@ -21,7 +21,7 @@ def get_http_proxy(): return default_proxy, no_proxy if os.path.isfile(proxy_config): - proxy_configdata = [line.strip() for line in check_output(proxy_config).decode("utf-8").splitlines()] + proxy_configdata = [line.strip() for line in check_output(["cat", proxy_config]).decode("utf-8").splitlines()] if len(proxy_configdata) >= 1: if not default_proxy: default_proxy = proxy_configdata[0] @@ -50,7 +50,7 @@ def merge(mapping: dict): class CommandSet: proxies = make_proxies(*get_http_proxy()) aliases = { - _: "env __SSH_PROGRAM_NAME__=%s %s" % (_, ssh_agent) + _: "env __SSH_PROGRAM_NAME__=%s -- %s" % (_, ssh_agent) for _ in ("ssh", "sftp", "scp", "slogin", "ssh-copy-id") } diff --git a/plugins/shell-proxy/ssh-agent.py b/plugins/shell-proxy/ssh-agent.py index 4ee24b755..79754f39a 100755 --- a/plugins/shell-proxy/ssh-agent.py +++ b/plugins/shell-proxy/ssh-agent.py @@ -8,7 +8,7 @@ ssh_proxy = os.path.join(os.path.dirname(__file__), "ssh-proxy.py") argv = [ os.environ.get("__SSH_PROGRAM_NAME__", "ssh"), "-o", - "ProxyCommand={} %h %p".format(ssh_proxy), + f"ProxyCommand={ssh_proxy} %h %p", "-o", "Compression=yes", ] diff --git a/plugins/shell-proxy/ssh-proxy.py b/plugins/shell-proxy/ssh-proxy.py index 4b692f9e4..9ff52e790 100755 --- a/plugins/shell-proxy/ssh-proxy.py +++ b/plugins/shell-proxy/ssh-proxy.py @@ -4,7 +4,10 @@ import subprocess import sys from urllib.parse import urlparse -proxy = next(os.environ[_] for _ in ("HTTP_PROXY", "HTTPS_PROXY") if _ in os.environ) +proxy = next((os.environ[_] for _ in ("HTTP_PROXY", "HTTPS_PROXY") if _ in os.environ), None) +if proxy is None: + sys.stderr.write("Error: Neither HTTP_PROXY nor HTTPS_PROXY environment variable is set\n") + sys.exit(1) parsed = urlparse(proxy) @@ -34,4 +37,4 @@ def make_argv(): yield sys.argv[1] # host yield sys.argv[2] # port -subprocess.call(make_argv()) +sys.exit(subprocess.call(make_argv()))