fix: CodeRabbit compliance — narrow exceptions, add compatible_versions#6
Open
5ymb01 wants to merge 3 commits intoChuckBuilds:masterfrom
Open
fix: CodeRabbit compliance — narrow exceptions, add compatible_versions#65ymb01 wants to merge 3 commits intoChuckBuilds:masterfrom
5ymb01 wants to merge 3 commits intoChuckBuilds:masterfrom
Conversation
config_manager.py: - Add _to_float() and _to_color() type coercion helpers to prevent TypeError/ValueError from string or malformed config values - Apply _to_float() to scroll_speed, scroll_delay, min_duration, max_duration, duration_buffer in both new-format and old-format paths - Apply _to_color() to all 6 color config reads (stock + crypto) - Narrow broad `except Exception` in _load_config, reload_config, validate_config to specific types (KeyError, TypeError, ValueError, AttributeError) - Add return type hints to __init__, _init_attributes data_fetcher.py: - Remove dead _extract_json_from_html() method (never called) - Replace stale web_interface_v2 try/except import with no-op stub - Remove unused `import re` - Add return type hint to cleanup() Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…nup traceback - Narrow 18 `except Exception` blocks to specific types across all .py files: - data_fetcher.py: 4 blocks → (requests.RequestException, ValueError, KeyError, TypeError) etc. - manager.py: 5 blocks → specific types + replace import traceback with logger.exception() (TRY401) - chart_renderer.py: 6 blocks → (AttributeError, TypeError, ValueError, ZeroDivisionError, OSError) - display_renderer.py: 3 blocks → (OSError, ValueError, TypeError, AttributeError) - Add `compatible_versions` to manifest.json (schema-required field) - Replace `import traceback` + manual format with `self.logger.exception()` (Ruff TRY401) Co-authored-by: 5ymb01 <5ymb01@users.noreply.github.com> Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughRefactors exception handling across multiple modules to narrower, explicit exception tuples; adds safe type-coercion helpers and restructuring to configuration loading; initializes an HTTP session and optional background service in the data fetcher; and adds a Changes
Sequence Diagram(s)sequenceDiagram
participant App
participant Fetcher as StockDataFetcher
participant BG as BackgroundService (optional)
participant API as External API
App->>Fetcher: fetch_all_data()
alt background service enabled
Fetcher->>BG: request_data()
BG->>API: fetch remote data
API-->>BG: data response
BG-->>Fetcher: data
else direct fetch
Fetcher->>API: HTTP request (session with retries)
API-->>Fetcher: data response
end
Fetcher->>Fetcher: parse & validate (specific exception handling)
Fetcher-->>App: return data / None on error
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
display_renderer.py (2)
156-158: Consider usinglogger.exception()for better debugging context.Static analysis suggests replacing
logger.error()withlogger.exception()when logging inside exception handlers. This automatically includes the stack trace, which aids debugging.♻️ Proposed fix
except (OSError, ValueError, TypeError, AttributeError) as e: - self.logger.error("Error loading font %s: %s, using default", font_name, e) + self.logger.exception("Error loading font %s, using default", font_name)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@display_renderer.py` around lines 156 - 158, Replace the call to self.logger.error inside the except block that catches (OSError, ValueError, TypeError, AttributeError) when loading fonts with self.logger.exception so the stack trace is included; locate the except that logs "Error loading font %s: %s, using default" (the one that captures variable e) and call self.logger.exception with a concise message like "Error loading font %s, using default" and the font_name so the exception and traceback are automatically recorded.
167-169: Consider usinglogger.exception()for consistency.Same as above—using
logger.exception()provides automatic stack traces in exception handlers.♻️ Proposed fix
except (OSError, ValueError, TypeError) as e: - self.logger.error("Error loading default font: %s", e) + self.logger.exception("Error loading default font") return ImageFont.load_default()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@display_renderer.py` around lines 167 - 169, The except block that currently calls self.logger.error("Error loading default font: %s", e) in the handler for except (OSError, ValueError, TypeError) as e should use self.logger.exception(...) to capture the full stack trace for consistency; replace the self.logger.error call with self.logger.exception("Error loading default font") while keeping the return ImageFont.load_default() behavior unchanged so callers still get a fallback font.data_fetcher.py (1)
101-102: Consider usinglogger.exception()for stack traces.Static analysis suggests using
logger.exception()instead oflogger.error()in exception handlers. However, for per-symbol errors in a loop, you may prefer the current approach to avoid verbose logs. If you want stack traces for debugging, consider:♻️ Optional: Use logger.exception for debugging
except (requests.RequestException, ValueError, KeyError, TypeError) as e: - self.logger.error("Error fetching stock data for %s: %s", symbol, e) + self.logger.exception("Error fetching stock data for %s", symbol)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@data_fetcher.py` around lines 101 - 102, The except block catching (requests.RequestException, ValueError, KeyError, TypeError) currently calls self.logger.error("Error fetching stock data for %s: %s", symbol, e); replace that with self.logger.exception("Error fetching stock data for %s", symbol) (or call self.logger.error(..., exc_info=True)) so the stack trace is recorded; if you want to avoid verbose traces for normal runs, gate the use of logger.exception/ exc_info=True behind a debug/config flag (use the same except block surrounding the symbol fetch logic in data_fetcher.py where self.logger.error is used).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@manager.py`:
- Around line 195-197: The except block declares an unused exception variable
`e`; update the exception handler for the block around self.logger.exception and
self.scroll_helper.clear_cache to drop the variable and use a bare except clause
with exception binding omitted (i.e., change "except (AttributeError,
ValueError, TypeError, OSError) as e:" to "except (AttributeError, ValueError,
TypeError, OSError):"), leaving the calls to self.logger.exception("Error
creating scrolling display") and self.scroll_helper.clear_cache() unchanged.
- Around line 104-105: The except block currently binds the exception to a
variable named e but never uses it; update the except clause in the method
containing the line with logger.exception("Error updating stock/crypto data") to
remove the unused variable (change "except (ValueError, AttributeError,
RuntimeError, TypeError) as e:" to "except (ValueError, AttributeError,
RuntimeError, TypeError):") so logger.exception(...) continues to record the
exception and stack trace without an unused variable.
---
Nitpick comments:
In `@data_fetcher.py`:
- Around line 101-102: The except block catching (requests.RequestException,
ValueError, KeyError, TypeError) currently calls self.logger.error("Error
fetching stock data for %s: %s", symbol, e); replace that with
self.logger.exception("Error fetching stock data for %s", symbol) (or call
self.logger.error(..., exc_info=True)) so the stack trace is recorded; if you
want to avoid verbose traces for normal runs, gate the use of logger.exception/
exc_info=True behind a debug/config flag (use the same except block surrounding
the symbol fetch logic in data_fetcher.py where self.logger.error is used).
In `@display_renderer.py`:
- Around line 156-158: Replace the call to self.logger.error inside the except
block that catches (OSError, ValueError, TypeError, AttributeError) when loading
fonts with self.logger.exception so the stack trace is included; locate the
except that logs "Error loading font %s: %s, using default" (the one that
captures variable e) and call self.logger.exception with a concise message like
"Error loading font %s, using default" and the font_name so the exception and
traceback are automatically recorded.
- Around line 167-169: The except block that currently calls
self.logger.error("Error loading default font: %s", e) in the handler for except
(OSError, ValueError, TypeError) as e should use self.logger.exception(...) to
capture the full stack trace for consistency; replace the self.logger.error call
with self.logger.exception("Error loading default font") while keeping the
return ImageFont.load_default() behavior unchanged so callers still get a
fallback font.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 8a36968a-bdb1-4712-9b29-52d05d96534e
📒 Files selected for processing (6)
chart_renderer.pyconfig_manager.pydata_fetcher.pydisplay_renderer.pymanager.pymanifest.json
logger.exception() automatically captures exception info, so the bound variable 'e' is unused. Removes Ruff F841 warnings. Co-Authored-By: 5ymb01 <5ymb01@users.noreply.github.com> Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
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.
Description
Addresses all CodeRabbit code quality findings across the stocks plugin codebase — narrows broad exception handlers, adds type-safe config parsing, removes dead code, and adds missing manifest fields.
Type of Change
Changes
manifest.jsoncompatible_versions, fixmin_ledmatrix_versionmanager.pyconfig_manager.py_to_float()/_to_color()helpers, narrow 4 broad exceptsdata_fetcher.py_extract_json_from_html()chart_renderer.pydisplay_renderer.pyCodeRabbit Compliance
except Exceptionblocks remainingimport tracebackstatementstraceback.format_exc()callscompatible_versionsin manifestTesting
Test Plan
Checklist
Author: @5ymb01
AI Assistant: Claude Opus 4.6 (Anthropic)
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes