Merged
Conversation
…nagers, adding build-dependencies for iterative-dev & establishing Contribution guidelines
📝 WalkthroughWalkthroughAdds packaging recipes and CI for multiple platforms, contributor docs and README tweaks, bumps project metadata, implements chafa provisioning fallback for non‑Kitty terminals, strengthens GitHub GraphQL error handling, and normalizes some exception/exports and runtime version lookup. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant CLI as hubfetch CLI
participant Auth as hubfetch.auth
participant Terminal as Terminal/Env
participant PkgMgr as Package Manager
participant Chafa as chafa binary
CLI->>Auth: request render (non‑kitty)
Auth->>Terminal: check PATH for chafa
Terminal-->>Auth: chafa missing
Auth->>Auth: _get_chafa_install_cmd() (probe pkg managers)
Auth->>User: prompt to install chafa (show command)
User-->>Auth: approve install
Auth->>PkgMgr: run install command (may invoke sudo)
PkgMgr-->>Auth: install success / failure
Auth->>Terminal: re-check PATH for chafa
Terminal-->>Auth: chafa found / still missing
Auth->>CLI: report success or fallback
CLI->>Chafa: if available, use chafa to render avatar
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (5)
hubfetch/auth.py (1)
122-130: Consider capturing subprocess output and adding a timeout.The subprocess call outputs directly to the terminal which can interleave with Click's output. A timeout prevents indefinite hangs if the package manager stalls.
Note: The static analysis S603 warning is a false positive here —
cmdis constructed from hardcoded values, not user input.Proposed improvement
try: - subprocess.run(cmd, check=True) + subprocess.run(cmd, check=True, timeout=300) except subprocess.CalledProcessError: click.echo( f"⚠ Failed to install chafa via {manager}.\n" " Please install it manually: https://hpjansson.org/chafa/download/", err=True, ) return False + except subprocess.TimeoutExpired: + click.echo( + f"⚠ Installation via {manager} timed out.\n" + " Please install chafa manually: https://hpjansson.org/chafa/download/", + err=True, + ) + return False🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@hubfetch/auth.py` around lines 122 - 130, The subprocess.run call that installs chafa should capture stdout/stderr and use a timeout to avoid hanging and prevent its output from interleaving with Click; update the subprocess.run invocation that uses the cmd variable to include capture_output=True (or stdout=subprocess.PIPE, stderr=subprocess.PIPE), text=True, and a sensible timeout (e.g., 30s), catch subprocess.TimeoutExpired in addition to subprocess.CalledProcessError, and include the captured stderr/stdout in the click.echo error messages before returning False so the failure details are visible (preserve existing behavior of returning False on error).hubfetch.spec (1)
11-23: Consider enabling bytecode optimization for release builds.
optimize=0keeps assert statements and docstrings. For smaller binaries, consideroptimize=2(removes asserts and docstrings).Note: The RuboCop self-assignment warnings are false positives — this is Python code using keyword arguments, not Ruby self-assignment.
Optional change for smaller binaries
excludes=[], noarchive=False, - optimize=0, + optimize=2, )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@hubfetch.spec` around lines 11 - 23, The spec sets PyInstaller Analysis with optimize=0 which retains asserts/docstrings increasing binary size; update the Analysis invocation (the Analysis(...) call that contains optimize=0) to set optimize=2 for release builds to remove asserts and docstrings, or make optimize configurable so tests/dev use 0 while CI/release uses 2.contrib/nix/package.nix (1)
1-1: Clarify the status of this packaging file.The comment says "abandoned for the time being" but the file is still in the repo and referenced in the PR. Consider either:
- Removing it entirely if truly abandoned, or
- Updating it to match the current version and removing the abandoned comment.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@contrib/nix/package.nix` at line 1, The top-line comment "mirror [abandoned for the time being]" in contrib/nix/package.nix is ambiguous and should be resolved: either remove the contrib/nix/package.nix file completely if it is no longer used, or update the package.nix contents to match the current packaging state and delete or replace the abandoned comment; ensure any references to package.nix in the PR or other build scripts are updated accordingly so nothing points at a removed/obsolete file.README.md (1)
48-52: Good addition documenting the chafa dependency.This aligns with the auto-installation logic in
auth.py. Consider a minor wording tweak for clarity:Optional wording improvement
-While the script does attempt to install in case terminal emulator appears to not support Kitty's graphics protocol, it is highly recommended to install manually in the event that chafa installation fails. +While the script attempts automatic installation when your terminal doesn't support Kitty's graphics protocol, manual installation is recommended if the automatic install fails.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@README.md` around lines 48 - 52, Update the "## Dependencies" section to clarify the relationship to auth.py's auto-install logic: mention that auth.py will attempt to install chafa if the terminal doesn't support Kitty graphics (referencing auth.py's auto-install flow), but because that can fail, strongly recommend users install Chafa manually beforehand; keep the Chafa link and the recommendation sentence, but rephrase to explicitly state "auth.py will try to auto-install chafa when Kitty support is absent, but manual installation is recommended if you want to avoid failures.".github/build.yml (1)
44-48: Updatesoftprops/action-gh-releasefromv1tov2.Version
v1is outdated. Versionv2(latest: v2.6.1) includes improvements and security fixes.Proposed update
- name: Create GitHub Release - uses: softprops/action-gh-release@v1 + uses: softprops/action-gh-release@v2 with: files: artifacts/* generate_release_notes: true🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/build.yml around lines 44 - 48, Update the GitHub Action step that creates the release: in the step named "Create GitHub Release" replace the uses identifier softprops/action-gh-release@v1 with softprops/action-gh-release@v2, keeping existing inputs (files: artifacts/* and generate_release_notes: true) and verify any v2-specific input/name changes if CI fails; ensure the step's uses string is the only change so it references the v2 release of the action.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/build.yml:
- Around line 1-2: The workflow file ".github/build.yml" is in the wrong
directory and won’t be picked up by GitHub Actions; move this file into the
workflows directory (rename/move to ".github/workflows/build.yml") so GitHub
recognizes it, preserving the existing content (including the name "Build
Binaries") and commit the change so the CI runs.
In `@contrib/nix/package.nix`:
- Line 10: The declared package version variable currently says version =
"1.0.2" but pyproject.toml uses 1.0.3; update the version variable to "1.0.3"
and then re-run the fetch (or nix-prefetch-url/fetchPypi) to compute the new
sha256, replacing the existing sha256 value (referenced on the line labelled
sha256) so the fetched tarball and checksum match the pyproject.toml version.
In `@CONTRIBUTING.md`:
- Line 7: Update the Python version mentioned in CONTRIBUTING.md to match the
interpreter constraint in pyproject.toml (or vice versa after deciding which
constraint to keep); locate the Python version string in CONTRIBUTING.md
("Python 3.10+ [Ideally better if you have 3.14 interpreter]") and the
requires-python field in pyproject.toml (e.g., "requires-python = \">=3.14\"")
and make them consistent (either change CONTRIBUTING.md to state "Python >=3.14"
or adjust pyproject.toml to your agreed minimum), then run a quick CI/tooling
check to ensure no other files reference a conflicting Python version.
In `@hubfetch/auth.py`:
- Around line 206-214: The display code only checks for chafa via
shutil.which("chafa") and never invokes the installer logic in _ensure_chafa(),
so users who skip `hubfetch auth` won't be prompted; refactor the chafa
detection/installation into a shared helper (e.g. move _ensure_chafa() from
auth.py into a common util like chafa_helper.ensure_chafa) and call that helper
from display.py before any rendering path (where shutil.which("chafa") is
currently checked) so installation prompts run when hubfetch is executed
directly; ensure the helper preserves the existing interactive behavior and
return values used by auth.py and display rendering logic.
In `@pyproject.toml`:
- Line 6: The pyproject.toml currently sets requires-python = ">=3.14" which
conflicts with CONTRIBUTING.md's "Python 3.10+" guidance; decide the intended
minimum and make them consistent: either change the pyproject.toml
requires-python value to ">=3.10" (or the chosen minimum like ">=3.11") or
update CONTRIBUTING.md to state "Python 3.14+"; update both the requires-python
entry in pyproject.toml and the Python-version statement in CONTRIBUTING.md to
match and add a brief note in CONTRIBUTING.md if the higher minimum is
intentional.
---
Nitpick comments:
In @.github/build.yml:
- Around line 44-48: Update the GitHub Action step that creates the release: in
the step named "Create GitHub Release" replace the uses identifier
softprops/action-gh-release@v1 with softprops/action-gh-release@v2, keeping
existing inputs (files: artifacts/* and generate_release_notes: true) and verify
any v2-specific input/name changes if CI fails; ensure the step's uses string is
the only change so it references the v2 release of the action.
In `@contrib/nix/package.nix`:
- Line 1: The top-line comment "mirror [abandoned for the time being]" in
contrib/nix/package.nix is ambiguous and should be resolved: either remove the
contrib/nix/package.nix file completely if it is no longer used, or update the
package.nix contents to match the current packaging state and delete or replace
the abandoned comment; ensure any references to package.nix in the PR or other
build scripts are updated accordingly so nothing points at a removed/obsolete
file.
In `@hubfetch.spec`:
- Around line 11-23: The spec sets PyInstaller Analysis with optimize=0 which
retains asserts/docstrings increasing binary size; update the Analysis
invocation (the Analysis(...) call that contains optimize=0) to set optimize=2
for release builds to remove asserts and docstrings, or make optimize
configurable so tests/dev use 0 while CI/release uses 2.
In `@hubfetch/auth.py`:
- Around line 122-130: The subprocess.run call that installs chafa should
capture stdout/stderr and use a timeout to avoid hanging and prevent its output
from interleaving with Click; update the subprocess.run invocation that uses the
cmd variable to include capture_output=True (or stdout=subprocess.PIPE,
stderr=subprocess.PIPE), text=True, and a sensible timeout (e.g., 30s), catch
subprocess.TimeoutExpired in addition to subprocess.CalledProcessError, and
include the captured stderr/stdout in the click.echo error messages before
returning False so the failure details are visible (preserve existing behavior
of returning False on error).
In `@README.md`:
- Around line 48-52: Update the "## Dependencies" section to clarify the
relationship to auth.py's auto-install logic: mention that auth.py will attempt
to install chafa if the terminal doesn't support Kitty graphics (referencing
auth.py's auto-install flow), but because that can fail, strongly recommend
users install Chafa manually beforehand; keep the Chafa link and the
recommendation sentence, but rephrase to explicitly state "auth.py will try to
auto-install chafa when Kitty support is absent, but manual installation is
recommended if you want to avoid failures."
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f10cc7c4-46d4-4967-9b64-3461233bf54e
⛔ Files ignored due to path filters (3)
assets/first_time.gifis excluded by!**/*.gifassets/first_time.mp4is excluded by!**/*.mp4uv.lockis excluded by!**/*.lock
📒 Files selected for processing (12)
.github/build.yml.gitignoreCONTRIBUTING.mdREADME.mdcontrib/aur/PKGBUILDcontrib/homebrew/homebrew.rbcontrib/nix/package.nixhubfetch.spechubfetch/auth.pyhubfetch/cache.pyhubfetch/config.pypyproject.toml
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (5)
hubfetch/cli.py (2)
7-7: Standard library import should be grouped with other stdlib imports.Per PEP 8 conventions, standard library imports (
importlib.metadata) should be grouped before third-party imports (click). Consider moving this import to the top alongsidesys.🧹 Suggested reordering
import sys +from importlib.metadata import version import click from hubfetch import config from hubfetch.auth import auth from hubfetch.api import GitHubClient from hubfetch.display import render -from importlib.metadata import version🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@hubfetch/cli.py` at line 7, Move the standard library import "from importlib.metadata import version" into the stdlib import group (with "import sys") so it appears before third-party imports like "import click"; locate the import of version in hubfetch/cli.py and reposition it above or alongside the "sys" import to follow PEP8 grouping of stdlib, third-party, then local imports.
11-11: Add fallback handling for uninstalled package scenarios.The
version("hubfetch")call is evaluated at module import time, not when--versionis invoked. If the package isn't installed (e.g., running from source during development withoutpip install -e .), this raisesPackageNotFoundErrorand crashes the CLI on import.Consider storing the version in a variable with error handling:
Proposed fallback handling
import sys +from importlib.metadata import version, PackageNotFoundError import click from hubfetch import config from hubfetch.auth import auth from hubfetch.api import GitHubClient from hubfetch.display import render -from importlib.metadata import version + +try: + __version__ = version("hubfetch") +except PackageNotFoundError: + __version__ = "dev" `@click.group`(invoke_without_command=True) `@click.pass_context` -@click.version_option(version=version("hubfetch")) +@click.version_option(version=__version__) def cli(ctx):🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@hubfetch/cli.py` at line 11, Compute the package version at import-time with a safe fallback: wrap the call to version("hubfetch") (the importlib.metadata.version call referenced in the click.version_option usage) in a try/except catching importlib.metadata.PackageNotFoundError (and optionally Exception) and assign a fallback string like "0.0.0" or "unknown" to a module-level variable (e.g., __version__), then pass that variable into click.version_option(version=__version__) so importing the CLI won’t raise when the package isn’t installed.hubfetch/api.py (1)
129-129: Consider removing informal comment.The local import to avoid circular dependencies is a valid pattern. However, the comment "A masterpiece of a local import by Sir Pranav" is informal and doesn't add value for future maintainers.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@hubfetch/api.py` at line 129, Remove the informal inline comment on the local import in hubfetch/api.py; locate the import statement that brings in ensure_avatar, get_cached_stats, save_stats from hubfetch.cache and delete the phrase "A masterpiece of a local import by Sir Pranav" so the line only contains the import, optionally adding a short neutral note like "# local import to avoid circular dependency" if a comment is needed for clarity.CONTRIBUTING.md (1)
48-55: Optional: Note platform differences in build output path.Line 54 uses Unix-style path notation (
./dist/hubfetch). Since the workflow builds for Windows too, Windows contributors will encounterdist\hubfetch.exe(ordist\hubfetch\hubfetch.exeif using directory bundles).📝 Optional clarification
```bash uv run pyinstaller hubfetch.spec -./dist/hubfetch +# Unix/macOS: ./dist/hubfetch +# Windows: dist\hubfetch.exeThis is a minor nitpick since most developers can adapt the path syntax for their platform. </details> <details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against the current code and only fix it if needed.
In
@CONTRIBUTING.mdaround lines 48 - 55, Update the "Building Locally" snippet
after the pyinstaller command to call out platform differences: replace or
augment the Unix-style path "./dist/hubfetch" with explicit examples for both
Unix/macOS and Windows (e.g., Unix/macOS: ./dist/hubfetch and Windows:
dist\hubfetch.exe or dist\hubfetch\hubfetch.exe for directory bundles) so
contributors on Windows know the expected output path after running "uv run
pyinstaller hubfetch.spec".</details> </blockquote></details> <details> <summary>.github/workflows/build.yml (1)</summary><blockquote> `21-22`: **Consider isolating build dependencies from dev tools.** The `uv sync` command installs all dependency groups, including dev tools like pytest, ruff, and twine (from `pyproject.toml` lines 20-28). While PyInstaller should only bundle runtime imports, having extra packages in the environment increases the risk of accidental inclusion and slightly increases setup time. <details> <summary>💡 Optional: Use production-only sync</summary> If you want to exclude dev dependencies during the build: ```diff - name: Install dependencies - run: uv sync + run: uv sync --no-devThen install PyInstaller separately:
+ - name: Install build tool + run: uv pip install pyinstaller + - name: Build binaryHowever, since PyInstaller is in your dev dependencies and the current approach works, this is optional.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/build.yml around lines 21 - 22, The workflow step named "Install dependencies" currently runs "uv sync" which installs all dependency groups including dev tools; change this to a production-only sync (so dev deps like PyInstaller, pytest, ruff, twine are excluded) and then, if the build needs PyInstaller, install PyInstaller explicitly in a separate step (or add it to the production install) so the environment used by the build contains only runtime deps; update the step that runs "uv sync" and add a follow-up step to install "PyInstaller" if required.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/build.yml:
- Around line 44-48: Update the GitHub Actions step named "Create GitHub
Release" to use softprops/action-gh-release@v2; locate the step that references
softprops/action-gh-release (the step name "Create GitHub Release") and change
the version tag from `@v1` to `@v2` while keeping the existing inputs (files:
artifacts/* and generate_release_notes: true) unchanged so the behavior and
configuration remain compatible with the v2 API.
In `@hubfetch/__init__.py`:
- Around line 1-3: The __all__ in hubfetch/__init__.py lists GitHubClient and
render but they are not exported from this module, breaking imports; fix by
either (A) importing those symbols into the package namespace (e.g., add
relative imports that bring GitHubClient and render into hubfetch/__init__.py
alongside config: import the actual definitions with from .<module> import
GitHubClient, render so they are available as hubfetch.GitHubClient and
hubfetch.render), or (B) if those symbols should not be package-level, remove
them from the __all__ list and keep only "config"; ensure the chosen approach
updates __all__ accordingly.
In `@hubfetch/api.py`:
- Around line 128-131: In fetch_stats, avoid using the falsy "or" fallback so
zero would be treated as missing; instead check explicitly for None: if year is
None then assign datetime.now().year to year. Update the assignment of the local
variable year inside fetch_stats (replace the "year = year or
datetime.now().year" pattern) to use an explicit None check so the function's
type safety and intent are clear.
- Around line 99-105: The RuntimeError raised when GraphQL returns errors or
missing user data should not crash the CLI; define a custom exception (e.g.,
HubFetchError or GraphQLError) in hubfetch.api and raise that instead of
RuntimeError inside the code that checks data (the block that currently raises
RuntimeError), include the original error details in the exception message, and
then update the CLI entry point (where fetch_stats() is awaited /
future_contribs.result() is called) to catch this custom exception and print a
user-friendly error message (and exit non-zero) rather than allowing a raw
traceback to surface.
In `@hubfetch/display.py`:
- Around line 48-50: The render path (ImageRenderer.render) must not call
hubfetch.auth._ensure_chafa because that helper can install packages and block;
change the logic so render only probes with shutil.which("chafa") and, if
missing, returns a safe fallback or raises a clear error instructing the user to
run an explicit setup step (e.g., call _ensure_chafa from an installer/setup
flow or a new opt-in flag). Concretely: remove the direct call to _ensure_chafa
from ImageRenderer.render, and either (a) raise a RuntimeError or custom
exception with guidance to run the installer helper, or (b) add an explicit
allow_install/init flag on ImageRenderer (e.g., allow_install=False) and only
call _ensure_chafa when that flag is true; keep all install logic confined to
hubfetch.auth._ensure_chafa or a separate setup command.
---
Nitpick comments:
In @.github/workflows/build.yml:
- Around line 21-22: The workflow step named "Install dependencies" currently
runs "uv sync" which installs all dependency groups including dev tools; change
this to a production-only sync (so dev deps like PyInstaller, pytest, ruff,
twine are excluded) and then, if the build needs PyInstaller, install
PyInstaller explicitly in a separate step (or add it to the production install)
so the environment used by the build contains only runtime deps; update the step
that runs "uv sync" and add a follow-up step to install "PyInstaller" if
required.
In `@CONTRIBUTING.md`:
- Around line 48-55: Update the "Building Locally" snippet after the pyinstaller
command to call out platform differences: replace or augment the Unix-style path
"./dist/hubfetch" with explicit examples for both Unix/macOS and Windows (e.g.,
Unix/macOS: ./dist/hubfetch and Windows: dist\hubfetch.exe or
dist\hubfetch\hubfetch.exe for directory bundles) so contributors on Windows
know the expected output path after running "uv run pyinstaller hubfetch.spec".
In `@hubfetch/api.py`:
- Line 129: Remove the informal inline comment on the local import in
hubfetch/api.py; locate the import statement that brings in ensure_avatar,
get_cached_stats, save_stats from hubfetch.cache and delete the phrase "A
masterpiece of a local import by Sir Pranav" so the line only contains the
import, optionally adding a short neutral note like "# local import to avoid
circular dependency" if a comment is needed for clarity.
In `@hubfetch/cli.py`:
- Line 7: Move the standard library import "from importlib.metadata import
version" into the stdlib import group (with "import sys") so it appears before
third-party imports like "import click"; locate the import of version in
hubfetch/cli.py and reposition it above or alongside the "sys" import to follow
PEP8 grouping of stdlib, third-party, then local imports.
- Line 11: Compute the package version at import-time with a safe fallback: wrap
the call to version("hubfetch") (the importlib.metadata.version call referenced
in the click.version_option usage) in a try/except catching
importlib.metadata.PackageNotFoundError (and optionally Exception) and assign a
fallback string like "0.0.0" or "unknown" to a module-level variable (e.g.,
__version__), then pass that variable into
click.version_option(version=__version__) so importing the CLI won’t raise when
the package isn’t installed.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 80985b48-ccbd-4fe5-8485-6e829841aca6
📒 Files selected for processing (8)
.github/workflows/build.yml.github/workflows/ci.ymlCONTRIBUTING.mdhubfetch/__init__.pyhubfetch/api.pyhubfetch/auth.pyhubfetch/cli.pyhubfetch/display.py
🚧 Files skipped from review as they are similar to previous changes (1)
- hubfetch/auth.py
Comment on lines
+99
to
+105
| if "errors" in data: | ||
| raise RuntimeError(f"GraphQL error: {data['errors']}") | ||
| user_data = data.get("data", {}).get("user") | ||
| if not user_data: | ||
| raise RuntimeError( | ||
| "Could not fetch contribution data. Check your token scopes." | ||
| ) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check for try/except blocks around fetch_stats or GitHubClient calls in CLI
rg -n -A5 -B2 'fetch_stats|GitHubClient' hubfetch/cli.pyRepository: PranavU-Coder/hubfetch
Length of output: 492
🏁 Script executed:
# Get full context of the CLI function
head -40 hubfetch/cli.pyRepository: PranavU-Coder/hubfetch
Length of output: 922
🏁 Script executed:
# Get the fetch_stats method implementation to verify ThreadPoolExecutor usage
rg -n -A 20 'def fetch_stats' hubfetch/api.pyRepository: PranavU-Coder/hubfetch
Length of output: 894
🏁 Script executed:
# Get more of the fetch_stats method to see how it handles futures and calls get_contributions
sed -n '128,170p' hubfetch/api.pyRepository: PranavU-Coder/hubfetch
Length of output: 1691
RuntimeError will propagate unhandled to CLI, causing unfriendly crash.
The improved error detection is good, but RuntimeError raised here will propagate through fetch_stats() (via future_contribs.result() at line 155) to the CLI entry point in cli.py, which has no exception handling. Users will see a raw traceback instead of a friendly error message.
Consider either:
- Catching these exceptions in
cli.pyand displaying user-friendly messages, or - Using a custom exception type that the CLI can specifically handle.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@hubfetch/api.py` around lines 99 - 105, The RuntimeError raised when GraphQL
returns errors or missing user data should not crash the CLI; define a custom
exception (e.g., HubFetchError or GraphQLError) in hubfetch.api and raise that
instead of RuntimeError inside the code that checks data (the block that
currently raises RuntimeError), include the original error details in the
exception message, and then update the CLI entry point (where fetch_stats() is
awaited / future_contribs.result() is called) to catch this custom exception and
print a user-friendly error message (and exit non-zero) rather than allowing a
raw traceback to surface.
Comment on lines
+48
to
+50
| if not shutil.which("chafa"): | ||
| from hubfetch.auth import _ensure_chafa | ||
| _ensure_chafa() |
There was a problem hiding this comment.
Don't install system packages from the render path.
Lines 48-50 make ImageRenderer.render() call _ensure_chafa(), but that helper is not just a probe: it can invoke a package manager, prompt for sudo, and write to the terminal. That is a risky side effect for a read-only display path and can hang or unexpectedly mutate the host during normal CLI usage. Keep rendering read-only here and move installation behind an explicit setup/auth flow or opt-in flag.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@hubfetch/display.py` around lines 48 - 50, The render path
(ImageRenderer.render) must not call hubfetch.auth._ensure_chafa because that
helper can install packages and block; change the logic so render only probes
with shutil.which("chafa") and, if missing, returns a safe fallback or raises a
clear error instructing the user to run an explicit setup step (e.g., call
_ensure_chafa from an installer/setup flow or a new opt-in flag). Concretely:
remove the direct call to _ensure_chafa from ImageRenderer.render, and either
(a) raise a RuntimeError or custom exception with guidance to run the installer
helper, or (b) add an explicit allow_install/init flag on ImageRenderer (e.g.,
allow_install=False) and only call _ensure_chafa when that flag is true; keep
all install logic confined to hubfetch.auth._ensure_chafa or a separate setup
command.
There was a problem hiding this comment.
@PranavU-Coder, glad you agree! Here's a quick summary of the recommended fix — adding an allow_install flag to ImageRenderer is probably the cleanest approach:
class ImageRenderer:
def __init__(self, size: int = 40, renderer: str = "auto", allow_install: bool = False):
self.width = size
self.height = size // 2
self.renderer = renderer
self.allow_install = allow_install
def render(self, image_path: str) -> Text | None:
if self.renderer == "none":
return None
if not shutil.which("chafa"):
if self.allow_install:
from hubfetch.auth import _ensure_chafa
_ensure_chafa()
else:
# chafa not available; fall through to placeholder
return self._placeholder()
if shutil.which("chafa"):
result = self._render_with_chafa(image_path)
if result:
return result
return self._placeholder()Then, in the auth/setup flow (e.g., hubfetch/auth.py), you can construct ImageRenderer(allow_install=True) when running the explicit setup step. The regular render() call in display.py stays with the default allow_install=False, keeping the render path side-effect-free.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
hubfetch/auth.py (1)
124-132: Consider adding a timeout to the subprocess call.The
subprocess.run(cmd, check=True)call has no timeout, which could cause the CLI to hang indefinitely if the package manager stalls (e.g., waiting for network, mirror issues).♻️ Suggested fix
try: - subprocess.run(cmd, check=True) + subprocess.run(cmd, check=True, timeout=300) # 5-minute timeout except subprocess.CalledProcessError: click.echo( f"⚠ Failed to install chafa via {manager}.\n" " Please install it manually: https://hpjansson.org/chafa/download/", err=True, ) return False + except subprocess.TimeoutExpired: + click.echo( + f"⚠ Installation timed out via {manager}.\n" + " Please install chafa manually: https://hpjansson.org/chafa/download/", + err=True, + ) + return False🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@hubfetch/auth.py` around lines 124 - 132, The subprocess.run call in the install flow (subprocess.run(cmd, check=True)) has no timeout and can hang; add a reasonable timeout argument (e.g., timeout=30 or configurable) to subprocess.run, catch subprocess.TimeoutExpired in addition to subprocess.CalledProcessError, ensure you terminate/cleanup as needed, and emit the same click.echo error message (including context) and return False on timeout; update the try/except block around subprocess.run to handle both exceptions and avoid indefinite CLI hangs.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@hubfetch/auth.py`:
- Around line 64-69: Remove the Chocolatey entry from the Windows package
managers list in the block that builds managers (the tuple ("choco", ["choco",
"install", "chafa", "-y"])); leave only the winget and scoop tuples so the code
no longer attempts to use Chocolatey to install chafa.
---
Nitpick comments:
In `@hubfetch/auth.py`:
- Around line 124-132: The subprocess.run call in the install flow
(subprocess.run(cmd, check=True)) has no timeout and can hang; add a reasonable
timeout argument (e.g., timeout=30 or configurable) to subprocess.run, catch
subprocess.TimeoutExpired in addition to subprocess.CalledProcessError, ensure
you terminate/cleanup as needed, and emit the same click.echo error message
(including context) and return False on timeout; update the try/except block
around subprocess.run to handle both exceptions and avoid indefinite CLI hangs.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 380bdcab-55ab-45f9-b608-70009d5475d5
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (6)
.github/workflows/ci.ymlhubfetch/__init__.pyhubfetch/api.pyhubfetch/auth.pyhubfetch/cli.pyhubfetch/display.py
💤 Files with no reviewable changes (1)
- hubfetch/init.py
✅ Files skipped from review due to trivial changes (2)
- hubfetch/cli.py
- .github/workflows/ci.yml
There was a problem hiding this comment.
🧹 Nitpick comments (1)
hubfetch/auth.py (1)
123-131: Consider adding a timeout to the subprocess call.The
subprocess.run()call lacks a timeout. If the package manager hangs due to network issues or unexpected prompts, the CLI will hang indefinitely.♻️ Proposed fix to add timeout
try: - subprocess.run(cmd, check=True) + subprocess.run(cmd, check=True, timeout=300) # 5 minute timeout except subprocess.CalledProcessError: click.echo( f"⚠ Failed to install chafa via {manager}.\n" " Please install it manually: https://hpjansson.org/chafa/download/", err=True, ) return False + except subprocess.TimeoutExpired: + click.echo( + f"⚠ Installation via {manager} timed out.\n" + " Please install chafa manually: https://hpjansson.org/chafa/download/", + err=True, + ) + return False🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@hubfetch/auth.py` around lines 123 - 131, Add a timeout to the subprocess.run call and handle timeouts: modify the try block around subprocess.run(cmd, check=True) in hubfetch/auth.py to pass a sensible timeout (e.g., timeout=60) and add an except subprocess.TimeoutExpired handler that calls click.echo with a clear message (similar to the CalledProcessError handler) and returns False; keep the existing CalledProcessError handling for install failures and reference the local variables cmd and manager in the timeout message so users know which installer hung.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@hubfetch/auth.py`:
- Around line 123-131: Add a timeout to the subprocess.run call and handle
timeouts: modify the try block around subprocess.run(cmd, check=True) in
hubfetch/auth.py to pass a sensible timeout (e.g., timeout=60) and add an except
subprocess.TimeoutExpired handler that calls click.echo with a clear message
(similar to the CalledProcessError handler) and returns False; keep the existing
CalledProcessError handling for install failures and reference the local
variables cmd and manager in the timeout message so users know which installer
hung.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d648e408-72d9-4a25-b5c3-7bffe8707ee7
📒 Files selected for processing (5)
.github/workflows/build.yml.github/workflows/ci.ymlhubfetch/__init__.pyhubfetch/auth.pyhubfetch/cli.py
💤 Files with no reviewable changes (1)
- .github/workflows/ci.yml
✅ Files skipped from review due to trivial changes (1)
- .github/workflows/build.yml
🚧 Files skipped from review as they are similar to previous changes (1)
- hubfetch/cli.py
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Chores