v2.5.35: 完善异常上抛机制 #409 #411
Conversation
WalkthroughThis pull request updates the project version and enhances error handling. The version string in the initialization file is incremented from 2.5.34 to 2.5.35. The API’s download function now accepts a new parameter to control exception checking. Additionally, the downloader’s exception handling has been refactored using a new decorator and segregated failure lists, and a new exception class has been introduced in the exceptions module. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as User
participant API as download_album()
participant Downloader as JmDownloader
Client->>API: Call download_album(jm_album_id, ..., check_exception=True)
API->>Downloader: Start download process
alt During download
Downloader->>Downloader: catch_exception decorator captures error
Downloader-->>Downloader: Record failure in image/photo list
end
API->>Downloader: Invoke raise_if_have_exception()
alt Failures detected
Downloader-->>API: Raise PartialDownloadFailedException
else No failures
Downloader-->>API: Return successful results
end
API-->>Client: Return download outcome
Possibly related PRs
Poem
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![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)
src/jmcomic/api.py (1)
62-62: Docstring alignment.
The docstring explanation forcheck_exceptionis concise. Ensure the rest of the documentation (e.g., README or usage docs) also references this new parameter.src/jmcomic/jm_downloader.py (2)
119-119: Decorator usage for image download.
@catch_exception('download_failed_image')similarly captures and logs all image failures. Keep an eye on memory usage if there are a large number of images, as these exception records will accumulate.
212-212: Consider checking photo failures inall_success.
Currently,if len(self.download_failed_image) != 0:setsall_success = False. For clarity, also checkingdownload_failed_photoexplicitly would make the logic more consistent.- if len(self.download_failed_image) != 0: + if len(self.download_failed_image) != 0 or len(self.download_failed_photo) != 0:
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/jmcomic/__init__.py(1 hunks)src/jmcomic/api.py(3 hunks)src/jmcomic/jm_downloader.py(6 hunks)src/jmcomic/jm_exception.py(2 hunks)
🧰 Additional context used
🧬 Code Definitions (2)
src/jmcomic/api.py (1)
src/jmcomic/jm_downloader.py (1)
raise_if_have_exception(282-289)
src/jmcomic/jm_downloader.py (4)
src/jmcomic/jm_entity.py (10)
JmImageDetail(169-259)JmPhotoDetail(262-421)is_image(23-24)is_image(258-259)download_url(209-218)is_photo(27-28)is_photo(420-421)id(67-68)id(403-404)id(482-483)src/jmcomic/jm_config.py (1)
jm_log(373-375)src/jmcomic/jm_client_interface.py (1)
download_by_image_detail(259-269)src/jmcomic/jm_exception.py (2)
raises(79-103)PartialDownloadFailedException(62-63)
🪛 GitHub Actions: Run Test (API)
src/jmcomic/jm_exception.py
[error] 103-103: raise e: 部分下载失败: 有0个章节下载失败, 0个图片下载失败
src/jmcomic/api.py
[error] 75-75: jmcomic.jm_exception.PartialDownloadFailedException: 部分下载失败: 有0个章节下载失败, 0个图片下载失败
src/jmcomic/jm_downloader.py
[error] 285-285: ExceptionTool.raises: 部分下载失败: 有0个章节下载失败, 0个图片下载失败
🪛 Ruff (0.8.2)
src/jmcomic/jm_downloader.py
56-56: List may be undefined, or defined from star imports
(F405)
56-56: Tuple may be undefined, or defined from star imports
(F405)
56-56: JmImageDetail may be undefined, or defined from star imports
(F405)
57-57: List may be undefined, or defined from star imports
(F405)
57-57: Tuple may be undefined, or defined from star imports
(F405)
57-57: JmPhotoDetail may be undefined, or defined from star imports
(F405)
67-67: JmBaseEntity may be undefined, or defined from star imports
(F405)
69-69: JmImageDetail may be undefined, or defined from star imports
(F405)
70-70: jm_log may be undefined, or defined from star imports
(F405)
73-73: JmPhotoDetail may be undefined, or defined from star imports
(F405)
74-74: jm_log may be undefined, or defined from star imports
(F405)
285-285: ExceptionTool may be undefined, or defined from star imports
(F405)
288-288: PartialDownloadFailedException may be undefined, or defined from star imports
(F405)
🔇 Additional comments (6)
src/jmcomic/__init__.py (1)
5-5: Version bump acknowledged.
The update from2.5.34to2.5.35aligns well with the enhanced error handling and exception changes elsewhere in the PR.src/jmcomic/jm_exception.py (1)
62-63: New exception class introduced.
DefiningPartialDownloadFailedExceptionis a good addition for handling partial failures. Ensure it’s documented so that users are aware of when this exception is raised and how to react to it.src/jmcomic/api.py (2)
74-75: Conditional raising of partial-failure exception.
Callingdler.raise_if_have_exception()only whencheck_exceptionisTrueprovides flexibility. Just confirm downstream callers handle the potentialPartialDownloadFailedExceptiongracefully.🧰 Tools
🪛 GitHub Actions: Run Test (API)
[error] 75-75: jmcomic.jm_exception.PartialDownloadFailedException: 部分下载失败: 有0个章节下载失败, 0个图片下载失败
51-52:Details
❓ Verification inconclusive
New parameter for optional exception checking.
Addingcheck_exception=Trueby default is convenient for robust error handling. However, confirm that existing code or users relying on silent failures won’t be unexpectedly broken by this new behavior.Could you verify any references to
download_album()in the codebase to ensure compatibility with the new default?
🏁 Script executed:
#!/bin/bash rg -A 5 'download_album\('Length of output: 6834
Action: Confirm new default exception checking behavior
The grep results confirm that all internal references to
download_album()(in tests and core modules such asjm_option.py,jm_plugin.py, etc.) do not currently pass an explicit value for the newcheck_exceptionparameter. This means they will now rely on the default ofTrue, potentially causing exceptions to be raised where failures were previously silent. Please verify that any external code or users expecting silent failure aren’t adversely affected by this change and update the documentation if needed.🧰 Tools
🪛 Ruff (0.8.2)
52-52:
Unionmay be undefined, or defined from star imports(F405)
52-52:
Setmay be undefined, or defined from star imports(F405)
src/jmcomic/jm_downloader.py (2)
105-105: Decorator usage for photo download.
@catch_exception('download_failed_photo')cleanly tracks failed chapter downloads. This approach helps maintain consistent error handling while keeping the primary download code uncluttered.
282-289: Raising partial download failure.
raise_if_have_exceptionis clear in its intent. Note that the pipeline logs show it sometimes triggers with zero failures, possibly when neither images nor successes exist. Double-check any edge cases where no items are actually downloaded to avoid raising a confusing error.🧰 Tools
🪛 Ruff (0.8.2)
285-285:
ExceptionToolmay be undefined, or defined from star imports(F405)
288-288:
PartialDownloadFailedExceptionmay be undefined, or defined from star imports(F405)
🪛 GitHub Actions: Run Test (API)
[error] 285-285: ExceptionTool.raises: 部分下载失败: 有0个章节下载失败, 0个图片下载失败
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/jmcomic/jm_downloader.py(7 hunks)
🧰 Additional context used
🧬 Code Definitions (1)
src/jmcomic/jm_downloader.py (4)
src/jmcomic/jm_entity.py (10)
JmImageDetail(169-259)JmPhotoDetail(262-421)is_image(23-24)is_image(258-259)download_url(209-218)is_photo(27-28)is_photo(420-421)id(67-68)id(403-404)id(482-483)src/jmcomic/jm_config.py (1)
jm_log(373-375)src/jmcomic/jm_client_interface.py (1)
download_by_image_detail(259-269)src/jmcomic/jm_exception.py (1)
PartialDownloadFailedException(62-63)
🪛 Ruff (0.8.2)
src/jmcomic/jm_downloader.py
56-56: List may be undefined, or defined from star imports
(F405)
56-56: Tuple may be undefined, or defined from star imports
(F405)
56-56: JmImageDetail may be undefined, or defined from star imports
(F405)
57-57: List may be undefined, or defined from star imports
(F405)
57-57: Tuple may be undefined, or defined from star imports
(F405)
57-57: JmPhotoDetail may be undefined, or defined from star imports
(F405)
67-67: JmBaseEntity may be undefined, or defined from star imports
(F405)
69-69: JmImageDetail may be undefined, or defined from star imports
(F405)
70-70: jm_log may be undefined, or defined from star imports
(F405)
73-73: JmPhotoDetail may be undefined, or defined from star imports
(F405)
74-74: jm_log may be undefined, or defined from star imports
(F405)
289-289: ExceptionTool may be undefined, or defined from star imports
(F405)
292-292: PartialDownloadFailedException may be undefined, or defined from star imports
(F405)
🪛 GitHub Actions: Run Test (API)
src/jmcomic/jm_downloader.py
[error] 105-105: TypeError: 'staticmethod' object is not callable
🔇 Additional comments (7)
src/jmcomic/jm_downloader.py (7)
56-57: Improved failure tracking with separate lists.Separating failed downloads by entity type enhances the diagnostic capabilities of the system. This allows for more precise error reporting and handling based on the specific type of failure.
🧰 Tools
🪛 Ruff (0.8.2)
56-56:
Listmay be undefined, or defined from star imports(F405)
56-56:
Tuplemay be undefined, or defined from star imports(F405)
56-56:
JmImageDetailmay be undefined, or defined from star imports(F405)
57-57:
Listmay be undefined, or defined from star imports(F405)
57-57:
Tuplemay be undefined, or defined from star imports(F405)
57-57:
JmPhotoDetailmay be undefined, or defined from star imports(F405)
139-143: Clean code refactoring of the download call.The code now uses a more direct method call which improves readability.
212-212: Correct update to use the new failure check property.The
all_successproperty now uses the newly introducedis_empty_download_failedproperty, which is the correct approach.
225-228: Good addition of helper property for failure checking.This property provides a clean interface to check if any downloads failed, simplifying code that needs this information.
286-294: Well-designed exception propagation mechanism.This method provides a convenient way to check for failures and raise an appropriate exception with detailed information. It follows the PR's objective of enhancing the exception propagation mechanism.
Consider adding more context about the specific failures in the exception message, such as IDs of failed entities, to help with debugging:
def raise_if_have_exception(self): if self.is_empty_download_failed: return + # Collect some sample failures to include in the message + photo_ids = [photo.id for photo, _ in self.download_failed_photo[:3]] if self.download_failed_photo else [] + image_urls = [img.download_url for img, _ in self.download_failed_image[:3]] if self.download_failed_image else [] ExceptionTool.raises( - f'部分下载失败: 有{len(self.download_failed_photo)}个章节下载失败, {len(self.download_failed_image)}个图片下载失败', + f'部分下载失败: 有{len(self.download_failed_photo)}个章节下载失败 (例如: {", ".join(photo_ids) if photo_ids else "无"}), ' + + f'{len(self.download_failed_image)}个图片下载失败 (例如: {", ".join(image_urls) if image_urls else "无"})', {'downloader': self}, PartialDownloadFailedException, )🧰 Tools
🪛 Ruff (0.8.2)
289-289:
ExceptionToolmay be undefined, or defined from star imports(F405)
292-292:
PartialDownloadFailedExceptionmay be undefined, or defined from star imports(F405)
105-105:⚠️ Potential issueFix decorator application syntax error.
The pipeline failure shows a
TypeError: 'staticmethod' object is not callableon this line.The decorator must be called with parentheses to create the actual decorator function:
- @catch_exception('download_failed_photo') + @catch_exception('download_failed_photo')The issue is likely with how the staticmethod is being accessed. Please ensure that you're using
JmDownloader.catch_exceptionproperly and that it returns a callable decorator.Likely an incorrect or invalid review comment.
🧰 Tools
🪛 GitHub Actions: Run Test (API)
[error] 105-105: TypeError: 'staticmethod' object is not callable
119-119:Details
✅ Verification successful
Ensure decorator works correctly for this method as well.
Similar to line 105, make sure the decorator application is syntactically correct here too.
🏁 Script executed:
#!/bin/bash # Check if other decorators in the codebase follow the same pattern rg -A 1 '@staticmethod\s+def\s+\w+\('Length of output: 39
Verify Decorator Consistency & Correct Application
Please ensure that the use of
@catch_exception('download_failed_image')on the method at line 119 adheres to the same syntactic and functional conventions as the decorator usage at line 105. In particular:
- Verify that the decorator correctly wraps the intended method.
- Confirm that the method defined immediately after the decorator is compatible with the decorator’s expected signature.
- Double-check for any subtle syntax differences compared to the similar implementation at line 105.
Manual review of the surrounding code context (see the shell script output) is recommended to rule out any integration issues.
🏁 Script executed:
#!/bin/bash echo "Extracting context for decorator usage in src/jmcomic/jm_downloader.py (lines 100-125):" sed -n '100,125p' src/jmcomic/jm_downloader.pyLength of output: 1309
Decorator Application Verified:
The code insrc/jmcomic/jm_downloader.pynow clearly shows that the decorator usage fordownload_by_image_detailis consistent with the pattern applied todownload_by_photo_detail(as seen at line 105). The decorator@catch_exception('download_failed_image')is syntactically correct and follows the established conventions in this file.No further changes are required.
src/jmcomic/jm_downloader.py
Outdated
| @staticmethod | ||
| def catch_exception(field_name): | ||
| def deco(func): | ||
| def wrapper(self, *args, **kwargs): | ||
| try: | ||
| return func(self, *args, **kwargs) | ||
| except Exception as e: | ||
| getattr(self, field_name).append(e) | ||
| detail: JmBaseEntity = args[1] | ||
| if detail.is_image(): | ||
| detail: JmImageDetail | ||
| jm_log('image.failed', f'图片下载失败: [{detail.download_url}], 异常: {e}') | ||
|
|
||
| elif detail.is_photo(): | ||
| detail: JmPhotoDetail | ||
| jm_log('photo.failed', f'章节下载失败: [{detail.id}], 异常: {e}') | ||
|
|
||
| raise e | ||
|
|
||
| return wrapper | ||
|
|
||
| return deco |
There was a problem hiding this comment.
Fix thread safety issues and tuple storage in the exception capture decorator.
The decorator correctly centralizes exception handling logic, but has two critical issues:
- It only stores the exception (
e) in line 66, but the type annotations in lines 56-57 indicate lists should store tuples of(DetailObject, Exception). - There's a potential thread-safety issue when multiple threads append to the shared lists concurrently.
Apply this diff to fix the issues:
@staticmethod
def catch_exception(field_name):
def deco(func):
def wrapper(self, *args, **kwargs):
try:
return func(self, *args, **kwargs)
except Exception as e:
- getattr(self, field_name).append(e)
+ detail: JmBaseEntity = args[1]
+ # Use thread-safe approach for appending to shared list
+ with getattr(self, '_lock', threading.RLock()):
+ if not hasattr(self, '_lock'):
+ setattr(self, '_lock', threading.RLock())
+ getattr(self, field_name).append((detail, e))
- detail: JmBaseEntity = args[1]
if detail.is_image():
detail: JmImageDetail
jm_log('image.failed', f'图片下载失败: [{detail.download_url}], 异常: {e}')Additionally, add the threading import at the top of the file:
import threading🧰 Tools
🪛 Ruff (0.8.2)
67-67: JmBaseEntity may be undefined, or defined from star imports
(F405)
69-69: JmImageDetail may be undefined, or defined from star imports
(F405)
70-70: jm_log may be undefined, or defined from star imports
(F405)
73-73: JmPhotoDetail may be undefined, or defined from star imports
(F405)
74-74: jm_log may be undefined, or defined from star imports
(F405)
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/jmcomic/jm_downloader.py (1)
4-24: 🛠️ Refactor suggestionGood refactoring for centralized exception handling, but thread safety concerns remain.
The new
catch_exceptiondecorator elegantly centralizes exception handling logic for both image and photo downloads. However, there's a potential thread-safety issue when multiple threads append to the shared lists concurrently.Consider implementing thread-safe list operations by adding a lock mechanism:
def catch_exception(field_name): def deco(func): def wrapper(self, *args, **kwargs): try: return func(self, *args, **kwargs) except Exception as e: detail: JmBaseEntity = args[1] + # Thread-safe approach for appending to shared list + if not hasattr(self, '_lock'): + from threading import RLock + setattr(self, '_lock', RLock()) + with getattr(self, '_lock'): getattr(self, field_name).append((detail, e)) if detail.is_image(): detail: JmImageDetail jm_log('image.failed', f'图片下载失败: [{detail.download_url}], 异常: {e}') elif detail.is_photo(): detail: JmPhotoDetail jm_log('photo.failed', f'章节下载失败: [{detail.id}], 异常: {e}') raise e return wrapper return deco🧰 Tools
🪛 Ruff (0.8.2)
10-10:
JmBaseEntitymay be undefined, or defined from star imports(F405)
13-13:
JmImageDetailmay be undefined, or defined from star imports(F405)
14-14:
jm_logmay be undefined, or defined from star imports(F405)
17-17:
JmPhotoDetailmay be undefined, or defined from star imports(F405)
18-18:
jm_logmay be undefined, or defined from star imports(F405)
🧹 Nitpick comments (1)
src/jmcomic/jm_downloader.py (1)
286-293: Robust implementation of the exception propagation mechanism.The
raise_if_have_exceptionmethod is a key part of the improved exception propagation mechanism. It clearly communicates the failure state by raising a specialized exception with detailed information about the failures.Consider adding more details to the exception for better debugging:
def raise_if_have_exception(self): if self.is_empty_download_failed: return ExceptionTool.raises( - f'部分下载失败: 有{len(self.download_failed_photo)}个章节下载失败, {len(self.download_failed_image)}个图片下载失败', + f'部分下载失败: 有{len(self.download_failed_photo)}个章节下载失败, {len(self.download_failed_image)}个图片下载失败。\n' + + f'失败章节IDs: {[photo.id for photo, _ in self.download_failed_photo][:5]}{"..." if len(self.download_failed_photo) > 5 else ""}\n' + + f'失败图片URLs: {[image.download_url for image, _ in self.download_failed_image][:5]}{"..." if len(self.download_failed_image) > 5 else ""}', {'downloader': self}, PartialDownloadFailedException, )🧰 Tools
🪛 Ruff (0.8.2)
289-289:
ExceptionToolmay be undefined, or defined from star imports(F405)
292-292:
PartialDownloadFailedExceptionmay be undefined, or defined from star imports(F405)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/jmcomic/jm_downloader.py(8 hunks)
🧰 Additional context used
🧬 Code Definitions (1)
src/jmcomic/jm_downloader.py (1)
src/jmcomic/jm_entity.py (10)
JmBaseEntity(16-36)is_image(23-24)is_image(258-259)JmImageDetail(169-259)is_photo(27-28)is_photo(420-421)JmPhotoDetail(262-421)id(67-68)id(403-404)id(482-483)
🪛 Ruff (0.8.2)
src/jmcomic/jm_downloader.py
10-10: JmBaseEntity may be undefined, or defined from star imports
(F405)
13-13: JmImageDetail may be undefined, or defined from star imports
(F405)
14-14: jm_log may be undefined, or defined from star imports
(F405)
17-17: JmPhotoDetail may be undefined, or defined from star imports
(F405)
18-18: jm_log may be undefined, or defined from star imports
(F405)
79-79: List may be undefined, or defined from star imports
(F405)
79-79: Tuple may be undefined, or defined from star imports
(F405)
79-79: JmImageDetail may be undefined, or defined from star imports
(F405)
80-80: List may be undefined, or defined from star imports
(F405)
80-80: Tuple may be undefined, or defined from star imports
(F405)
80-80: JmPhotoDetail may be undefined, or defined from star imports
(F405)
289-289: ExceptionTool may be undefined, or defined from star imports
(F405)
292-292: PartialDownloadFailedException may be undefined, or defined from star imports
(F405)
🔇 Additional comments (6)
src/jmcomic/jm_downloader.py (6)
79-80: Improved error tracking through separate failure lists.Splitting the failed downloads into separate lists for images and photos enhances clarity and makes error reporting more precise. This is a good improvement in the error handling mechanism.
🧰 Tools
🪛 Ruff (0.8.2)
79-79:
Listmay be undefined, or defined from star imports(F405)
79-79:
Tuplemay be undefined, or defined from star imports(F405)
79-79:
JmImageDetailmay be undefined, or defined from star imports(F405)
80-80:
Listmay be undefined, or defined from star imports(F405)
80-80:
Tuplemay be undefined, or defined from star imports(F405)
80-80:
JmPhotoDetailmay be undefined, or defined from star imports(F405)
105-105: Decorator application simplifies code.Applying the
catch_exceptiondecorator to the method reduces code duplication and centralizes the error handling logic, which is a good practice.
119-119: Consistent decorator application across methods.Using the same error handling pattern consistently for both photo and image details improves code maintainability and readability.
139-143: Client method call parameters update.The
download_by_image_detailmethod call has been updated with more explicit parameter names, which improves code readability.
212-212: Code readability improvement through property extraction.Using the new
is_empty_download_failedproperty improves code readability by abstracting the empty check logic.
225-228: Well-designed property for failure list checking.This property effectively encapsulates the logic for checking if both failure lists are empty, making the code more maintainable and reducing duplication.
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
src/jmcomic/jm_downloader.py (1)
321-324: 🛠️ Refactor suggestionUpdate subclass implementations to use the decorator
The subclasses
DoNotDownloadImageandJustDownloadSpecificCountImageoverridedownload_by_image_detailbut don't apply the new@catch_exceptiondecorator. This could lead to inconsistent exception handling.Update the subclass methods to match the parent's implementation style:
class DoNotDownloadImage(JmDownloader): """ 不会下载任何图片的Downloader，用作测试 """ + @catch_exception('download_failed_image') def download_by_image_detail(self, image: JmImageDetail, client: JmcomicClient): # ensure make dir self.option.decide_image_filepath(image)class JustDownloadSpecificCountImage(JmDownloader): """ 只下载特定数量图片的Downloader，用作测试 """ from threading import Lock count_lock = Lock() count = 0 + @catch_exception('download_failed_image') def download_by_image_detail(self, image: JmImageDetail, client: JmcomicClient): # ensure make dir self.option.decide_image_filepath(image) if self.try_countdown(): return super().download_by_image_detail(image, client)Also applies to: 335-341
🧰 Tools
🪛 Ruff (0.8.2)
321-321:
JmImageDetailmay be undefined, or defined from star imports(F405)
321-321:
JmcomicClientmay be undefined, or defined from star imports(F405)
🧹 Nitpick comments (1)
src/jmcomic/jm_downloader.py (1)
1-1: Consider explicit imports for better code maintainabilityThe code uses star imports (
from .jm_option import *), which is generally discouraged as it makes it harder to track where symbols are coming from. This is evident from the static analysis warnings about undefined symbols.Consider using explicit imports for clarity:
-from .jm_option import * +from .jm_option import JmOption, JmModuleConfig, file_exists, JmcomicClient +from .jm_entity import JmBaseEntity, JmImageDetail, JmPhotoDetail, JmAlbumDetail, DetailEntity +from .jm_exception import ExceptionTool, PartialDownloadFailedException +from .jm_config import jm_log +from typing import Dict, List, Tuple, Callable +import threading🧰 Tools
🪛 Ruff (0.8.2)
1-1:
from .jm_option import *used; unable to detect undefined names(F403)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/jmcomic/jm_downloader.py(8 hunks)
🧰 Additional context used
🧬 Code Definitions (1)
src/jmcomic/jm_downloader.py (4)
src/jmcomic/jm_entity.py (11)
JmBaseEntity(16-36)is_image(23-24)is_image(258-259)JmImageDetail(169-259)download_url(209-218)is_photo(27-28)is_photo(420-421)JmPhotoDetail(262-421)id(67-68)id(403-404)id(482-483)src/jmcomic/jm_config.py (1)
jm_log(373-375)src/jmcomic/jm_client_interface.py (1)
download_by_image_detail(259-269)src/jmcomic/jm_exception.py (3)
ExceptionTool(66-187)raises(79-103)PartialDownloadFailedException(62-63)
🪛 Ruff (0.8.2)
src/jmcomic/jm_downloader.py
10-10: JmBaseEntity may be undefined, or defined from star imports
(F405)
13-13: JmImageDetail may be undefined, or defined from star imports
(F405)
14-14: jm_log may be undefined, or defined from star imports
(F405)
17-17: JmPhotoDetail may be undefined, or defined from star imports
(F405)
18-18: jm_log may be undefined, or defined from star imports
(F405)
79-79: List may be undefined, or defined from star imports
(F405)
79-79: Tuple may be undefined, or defined from star imports
(F405)
79-79: JmImageDetail may be undefined, or defined from star imports
(F405)
80-80: List may be undefined, or defined from star imports
(F405)
80-80: Tuple may be undefined, or defined from star imports
(F405)
80-80: JmPhotoDetail may be undefined, or defined from star imports
(F405)
289-289: ExceptionTool may be undefined, or defined from star imports
(F405)
294-294: PartialDownloadFailedException may be undefined, or defined from star imports
(F405)
🔇 Additional comments (4)
src/jmcomic/jm_downloader.py (4)
79-80: Good separation of concerns in tracking failuresSeparating the failure lists for images and photos improves clarity and makes it easier to handle different types of failures. The type annotations correctly indicate that each list holds tuples of (DetailObject, Exception).
🧰 Tools
🪛 Ruff (0.8.2)
79-79:
Listmay be undefined, or defined from star imports(F405)
79-79:
Tuplemay be undefined, or defined from star imports(F405)
79-79:
JmImageDetailmay be undefined, or defined from star imports(F405)
80-80:
Listmay be undefined, or defined from star imports(F405)
80-80:
Tuplemay be undefined, or defined from star imports(F405)
80-80:
JmPhotoDetailmay be undefined, or defined from star imports(F405)
105-105: Consistent application of exception handlingApplying the decorator to both download methods ensures consistent exception handling throughout the codebase.
Also applies to: 119-119
225-228: Clear and concise property for checking failed downloadsThe
is_empty_download_failedproperty provides a clean interface for checking if all downloads were successful.
286-296: Effective exception propagation mechanismThe
raise_if_have_exceptionmethod effectively implements the PR's goal of enhancing exception propagation. It provides a comprehensive error message that includes the count of failed downloads and examples of the failures, which will be helpful for debugging.🧰 Tools
🪛 Ruff (0.8.2)
289-289:
ExceptionToolmay be undefined, or defined from star imports(F405)
294-294:
PartialDownloadFailedExceptionmay be undefined, or defined from star imports(F405)
| def catch_exception(field_name): | ||
| def deco(func): | ||
| def wrapper(self, *args, **kwargs): | ||
| try: | ||
| return func(self, *args, **kwargs) | ||
| except Exception as e: | ||
| detail: JmBaseEntity = args[1] | ||
| getattr(self, field_name).append((detail, e)) | ||
| if detail.is_image(): | ||
| detail: JmImageDetail | ||
| jm_log('image.failed', f'图片下载失败: [{detail.download_url}], 异常: {e}') | ||
|
|
||
| elif detail.is_photo(): | ||
| detail: JmPhotoDetail | ||
| jm_log('photo.failed', f'章节下载失败: [{detail.id}], 异常: {e}') | ||
|
|
||
| raise e | ||
|
|
||
| return wrapper | ||
|
|
||
| return deco |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Centralized exception handling with decorator pattern is a good improvement
The new catch_exception decorator effectively centralizes the exception handling logic, making the code more maintainable and consistent. It properly captures both the detail object and the exception, and logs appropriate messages based on the object type.
However, there's a potential thread safety issue when multiple threads append to the shared lists concurrently.
Consider adding thread safety protection:
def catch_exception(field_name):
def deco(func):
def wrapper(self, *args, **kwargs):
try:
return func(self, *args, **kwargs)
except Exception as e:
detail: JmBaseEntity = args[1]
+ # Use thread-safe approach for appending to shared list
+ with getattr(self, '_lock', threading.RLock()):
+ if not hasattr(self, '_lock'):
+ setattr(self, '_lock', threading.RLock())
getattr(self, field_name).append((detail, e))
if detail.is_image():
detail: JmImageDetail
jm_log('image.failed', f'图片下载失败: [{detail.download_url}], 异常: {e}')
elif detail.is_photo():
detail: JmPhotoDetail
jm_log('photo.failed', f'章节下载失败: [{detail.id}], 异常: {e}')
raise e
return wrapper
return decoDon't forget to import threading at the top of the file:
import threading🧰 Tools
🪛 Ruff (0.8.2)
10-10: JmBaseEntity may be undefined, or defined from star imports
(F405)
13-13: JmImageDetail may be undefined, or defined from star imports
(F405)
14-14: jm_log may be undefined, or defined from star imports
(F405)
17-17: JmPhotoDetail may be undefined, or defined from star imports
(F405)
18-18: jm_log may be undefined, or defined from star imports
(F405)
Summary by CodeRabbit
Chores
New Features
Refactor