v2.5.22: 新增long_img插件，将本子合并为长图(#294) #295
Conversation
WalkthroughThe changes in this pull request involve multiple updates to the JMComic Python API documentation and codebase. Key modifications include enhancements to the Changes
Possibly related PRs
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 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: 7
🧹 Outside diff range and nitpick comments (3)
README.md (2)
Line range hint
1-46: LGTM! Clear project introduction and installation instructions.The expanded project introduction and updated installation instructions provide a comprehensive overview for users. The addition of the Python version requirement (>=3.7) is particularly helpful.
Consider adding a brief note about potential dependencies or system requirements beyond Python itself, if any exist.
Line range hint
48-83: LGTM! Comprehensive usage guide with clear examples.The quick start guide and configuration file usage instructions are well-written and provide users with a clear path to get started with the project.
Consider adding a brief example of a common use case for the
optionparameter in thedownload_albumfunction to illustrate its practical application.assets/docs/sources/option_file_syntax.md (1)
247-252: LGTM! Consider adding a note about potential file size limitations.The addition of the
long_imgplugin is well-documented and provides a valuable alternative to the existingimg2pdfplugin. The placement and configuration are consistent with other plugins in the documentation.Consider adding a note about potential file size limitations when merging all images into a single PNG, especially for albums with many high-resolution images. This could help users make an informed decision between using
long_imgandimg2pdf.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
- README.md (1 hunks)
- assets/docs/sources/option_file_syntax.md (1 hunks)
- src/jmcomic/init.py (1 hunks)
- src/jmcomic/jm_plugin.py (1 hunks)
✅ Files skipped from review due to trivial changes (1)
- src/jmcomic/init.py
🧰 Additional context used
🪛 Ruff
src/jmcomic/jm_plugin.py
806-806:
JmPhotoDetailmay be undefined, or defined from star imports(F405)
807-807:
JmAlbumDetailmay be undefined, or defined from star imports(F405)
815-815:
jm_logmay be undefined, or defined from star imports(F405)
818-818:
PIL.Imageimported but unused; consider usingimportlib.util.find_specto test for availability(F401)
829-829:
DirRulemay be undefined, or defined from star imports(F405)
841-841:
JmAlbumDetailmay be undefined, or defined from star imports(F405)
841-841:
JmPhotoDetailmay be undefined, or defined from star imports(F405)
841-841:
Listmay be undefined, or defined from star imports(F405)
850-850:
files_of_dirmay be undefined, or defined from star imports(F405)
880-880:
Optionalmay be undefined, or defined from star imports(F405)
881-881:
fix_filepathmay be undefined, or defined from star imports(F405)
882-882:
mkdir_if_not_existsmay be undefined, or defined from star imports(F405)
🔇 Additional comments (3)
README.md (3)
Line range hint
85-91: LGTM! Clear direction for advanced usage.The link to the project documentation and the note about available configuration options provide a good pathway for users to explore more advanced features of the project.
Line range hint
1-128: Overall, excellent improvements to the README.md file.The changes made to the README.md file significantly enhance the documentation. The expanded project introduction, clear installation instructions, comprehensive usage guide, and detailed feature list provide users with a thorough understanding of the project and its capabilities.
Line range hint
93-128: LGTM! Comprehensive list of project features and plugins.The expanded list of features and plugins provides an excellent overview of the project's capabilities. The addition of new plugins for merging images into PDF files and creating long images enhances the project's functionality.
Let's verify the implementation of the new plugins:
| photo: JmPhotoDetail = None, | ||
| album: JmAlbumDetail = None, |
There was a problem hiding this comment.
Explicitly import JmPhotoDetail and JmAlbumDetail to avoid reliance on star imports
JmPhotoDetail and JmAlbumDetail may be undefined if they are not imported explicitly. Relying on star imports (from ... import *) is discouraged as it can lead to unexpected behaviors and makes the code less readable and maintainable.
Add the following imports at the top of the file:
from .jm_model import JmPhotoDetail, JmAlbumDetail🧰 Tools
🪛 Ruff
806-806:
JmPhotoDetailmay be undefined, or defined from star imports(F405)
807-807:
JmAlbumDetailmay be undefined, or defined from star imports(F405)
| **kwargs, | ||
| ): | ||
| if photo is None and album is None: | ||
| jm_log('wrong_usage', 'long_img必须运行在after_photo或after_album时') |
There was a problem hiding this comment.
Explicitly import jm_log to ensure it is defined
The function jm_log may be undefined if it's not imported explicitly. Relying on star imports can cause issues and reduce code clarity. Explicit imports improve code readability and prevent potential NameError exceptions.
Add the following import at the top of the file:
from .jm_logger import jm_log🧰 Tools
🪛 Ruff
815-815:
jm_logmay be undefined, or defined from star imports(F405)
| img_dir = self.get_img_dir(img_dir) | ||
|
|
||
| # 处理生成的长图文件的路径 | ||
| filename = DirRule.apply_rule_directly(album, photo, filename_rule) |
There was a problem hiding this comment.
Explicitly import DirRule to ensure it is defined
DirRule may be undefined if not imported directly. Explicitly importing DirRule helps avoid potential runtime errors and enhances code clarity.
Add the following import at the top of the file:
from .dir_rule import DirRule🧰 Tools
🪛 Ruff
829-829:
DirRulemay be undefined, or defined from star imports(F405)
| # 执行删除 | ||
| self.execute_deletion(img_path_ls) | ||
|
|
||
| def write_img_2_long_img(self, long_img_path, album: JmAlbumDetail, photo: JmPhotoDetail) -> List[str]: |
There was a problem hiding this comment.
Explicitly import JmAlbumDetail, JmPhotoDetail, and List
The types JmAlbumDetail, JmPhotoDetail, and List may be undefined if not imported explicitly. This can lead to NameError exceptions and reduces code readability.
Add the following imports at the top of the file:
from typing import List, Optional
from .jm_model import JmAlbumDetail, JmPhotoDetail🧰 Tools
🪛 Ruff
841-841:
JmAlbumDetailmay be undefined, or defined from star imports(F405)
841-841:
JmPhotoDetailmay be undefined, or defined from star imports(F405)
841-841:
Listmay be undefined, or defined from star imports(F405)
src/jmcomic/jm_plugin.py
Outdated
| else: | ||
| img_dir_items = [self.option.decide_image_save_dir(photo) for photo in album] | ||
|
|
||
| img_paths = list(itertools.chain(*map(files_of_dir, img_dir_items))) |
There was a problem hiding this comment.
Explicitly import files_of_dir to ensure it is defined
The function files_of_dir may be undefined if not imported explicitly. Importing it directly ensures that it's available when needed and prevents potential NameError exceptions.
Add the following import at the top of the file:
from common import files_of_dir🧰 Tools
🪛 Ruff
850-850:
files_of_dirmay be undefined, or defined from star imports(F405)
src/jmcomic/jm_plugin.py
Outdated
| y_offset += img.height | ||
|
|
||
| long_img.save(long_img_path) | ||
| [img.close() for img in images] |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use a for-loop instead of a list comprehension for side-effects
Using a list comprehension solely for its side effects is not recommended, as it creates an unnecessary list and can be confusing to readers. It's clearer and more efficient to use a simple for loop when the result of the comprehension is not needed.
Change:
[img.close() for img in images]To:
for img in images:
img.close()| img_dir = fix_filepath(img_dir or os.getcwd()) | ||
| mkdir_if_not_exists(img_dir) |
There was a problem hiding this comment.
Explicitly import fix_filepath and mkdir_if_not_exists
The functions fix_filepath and mkdir_if_not_exists may be undefined if not imported explicitly. Importing them ensures they are available when needed and enhances code clarity.
Add the following imports at the top of the file:
from .jm_utils import fix_filepath, mkdir_if_not_exists🧰 Tools
🪛 Ruff
881-881:
fix_filepathmay be undefined, or defined from star imports(F405)
882-882:
mkdir_if_not_existsmay be undefined, or defined from star imports(F405)
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (1)
src/jmcomic/jm_plugin.py (1)
891-895: Minor improvements forget_img_dirmethodThe
get_img_dirmethod is concise and functional. Consider the following minor improvements:
- Add a docstring to explain the method's purpose and parameters.
- Use an f-string for better readability when joining the current working directory.
Here's a suggested improvement:
@staticmethod def get_img_dir(img_dir: Optional[str]) -> str: """ Ensure the image directory exists and return its path. :param img_dir: Optional path to the image directory. If None, uses the current working directory. :return: The absolute path to the image directory. """ img_dir = fix_filepath(img_dir or f"{os.getcwd()}/") mkdir_if_not_exists(img_dir) return img_dirThese changes improve the method's documentation and readability.
🧰 Tools
🪛 Ruff
891-891:
Optionalmay be undefined, or defined from star imports(F405)
892-892:
fix_filepathmay be undefined, or defined from star imports(F405)
893-893:
mkdir_if_not_existsmay be undefined, or defined from star imports(F405)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- src/jmcomic/jm_plugin.py (1 hunks)
🧰 Additional context used
🪛 Ruff
src/jmcomic/jm_plugin.py
806-806:
JmPhotoDetailmay be undefined, or defined from star imports(F405)
807-807:
JmAlbumDetailmay be undefined, or defined from star imports(F405)
815-815:
jm_logmay be undefined, or defined from star imports(F405)
818-818:
PIL.Imageimported but unused; consider usingimportlib.util.find_specto test for availability(F401)
829-829:
DirRulemay be undefined, or defined from star imports(F405)
841-841:
JmAlbumDetailmay be undefined, or defined from star imports(F405)
841-841:
JmPhotoDetailmay be undefined, or defined from star imports(F405)
841-841:
Listmay be undefined, or defined from star imports(F405)
850-850:
files_of_dirmay be undefined, or defined from star imports(F405)
880-880:
Listmay be undefined, or defined from star imports(F405)
884-884:
Imagemay be undefined, or defined from star imports(F405)
891-891:
Optionalmay be undefined, or defined from star imports(F405)
892-892:
fix_filepathmay be undefined, or defined from star imports(F405)
893-893:
mkdir_if_not_existsmay be undefined, or defined from star imports(F405)
🔇 Additional comments (2)
src/jmcomic/jm_plugin.py (2)
802-804: LGTM: Class definition looks goodThe
LongImgPluginclass is correctly defined as a subclass ofJmOptionPlugin. Theplugin_keyis appropriately set to 'long_img'.
802-895: Overall assessment of LongImgPluginThe
LongImgPluginclass is a well-implemented feature for creating long images from multiple source images. It demonstrates good use of object-oriented programming principles and effectively utilizes the PIL library for image processing.Key strengths:
- Flexible handling of both single photos and albums
- Efficient image resizing to maintain aspect ratios
- Clear separation of concerns across multiple methods
Areas for improvement:
- Enhanced error handling and logging
- Better resource management using context managers
- More explicit imports and type hinting
These improvements will make the code more robust, efficient, and maintainable. Great job on implementing this useful feature!
🧰 Tools
🪛 Ruff
806-806:
JmPhotoDetailmay be undefined, or defined from star imports(F405)
807-807:
JmAlbumDetailmay be undefined, or defined from star imports(F405)
815-815:
jm_logmay be undefined, or defined from star imports(F405)
818-818:
PIL.Imageimported but unused; consider usingimportlib.util.find_specto test for availability(F401)
829-829:
DirRulemay be undefined, or defined from star imports(F405)
841-841:
JmAlbumDetailmay be undefined, or defined from star imports(F405)
841-841:
JmPhotoDetailmay be undefined, or defined from star imports(F405)
841-841:
Listmay be undefined, or defined from star imports(F405)
850-850:
files_of_dirmay be undefined, or defined from star imports(F405)
880-880:
Listmay be undefined, or defined from star imports(F405)
884-884:
Imagemay be undefined, or defined from star imports(F405)
891-891:
Optionalmay be undefined, or defined from star imports(F405)
892-892:
fix_filepathmay be undefined, or defined from star imports(F405)
893-893:
mkdir_if_not_existsmay be undefined, or defined from star imports(F405)
| def invoke(self, | ||
| photo: JmPhotoDetail = None, | ||
| album: JmAlbumDetail = None, | ||
| downloader=None, | ||
| img_dir=None, | ||
| filename_rule='Pid', | ||
| delete_original_file=False, | ||
| **kwargs, | ||
| ): | ||
| if photo is None and album is None: | ||
| jm_log('wrong_usage', 'long_img必须运行在after_photo或after_album时') |
There was a problem hiding this comment.
Address import and error handling in the invoke method
- The
JmPhotoDetailandJmAlbumDetailimports should be explicit to avoid potential issues with star imports. - Consider adding a more informative error message when both
photoandalbumare None. - The PIL import could be moved to the top of the file for better organization.
Here's a suggested improvement for the imports and error handling:
from .jm_model import JmPhotoDetail, JmAlbumDetail
from .jm_logger import jm_log
from typing import Optional
# At the top of the file
try:
from PIL import Image
except ImportError:
Image = None
class LongImgPlugin(JmOptionPlugin):
# ...
def invoke(self,
photo: Optional[JmPhotoDetail] = None,
album: Optional[JmAlbumDetail] = None,
# ...
):
if photo is None and album is None:
raise ValueError("Either 'photo' or 'album' must be provided")
if Image is None:
self.warning_lib_not_install('Pillow')
return
# Rest of the method...This change improves type hinting, error handling, and import organization.
🧰 Tools
🪛 Ruff
806-806:
JmPhotoDetailmay be undefined, or defined from star imports(F405)
807-807:
JmAlbumDetailmay be undefined, or defined from star imports(F405)
815-815:
jm_logmay be undefined, or defined from star imports(F405)
src/jmcomic/jm_plugin.py
Outdated
| def write_img_2_long_img(self, long_img_path, album: JmAlbumDetail, photo: JmPhotoDetail) -> List[str]: | ||
| import itertools | ||
| from PIL import Image | ||
|
|
||
| if album is None: | ||
| img_dir_items = [self.option.decide_image_save_dir(photo)] | ||
| else: | ||
| img_dir_items = [self.option.decide_image_save_dir(photo) for photo in album] | ||
|
|
||
| img_paths = list(itertools.chain(*map(files_of_dir, img_dir_items))) | ||
| img_paths = sorted(img_paths) | ||
|
|
||
| images = self.open_images(img_paths) | ||
|
|
||
| try: | ||
| resample_method = Image.Resampling.LANCZOS | ||
| except AttributeError: | ||
| resample_method = Image.LANCZOS | ||
|
|
||
| min_img_width = min(img.width for img in images) | ||
| total_height = 0 | ||
| for i, img in enumerate(images): | ||
| if img.width > min_img_width: | ||
| images[i] = img.resize((min_img_width, int(img.height * min_img_width / img.width)), | ||
| resample=resample_method) | ||
| total_height += images[i].height | ||
|
|
||
| long_img = Image.new('RGB', (min_img_width, total_height)) | ||
| y_offset = 0 | ||
| for img in images: | ||
| long_img.paste(img, (0, y_offset)) | ||
| y_offset += img.height | ||
|
|
||
| long_img.save(long_img_path) | ||
| for img in images: | ||
| img.close() | ||
|
|
||
| return img_paths | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Optimize and improve error handling in write_img_2_long_img
The method is well-implemented, but consider the following improvements:
- Use a context manager for opening and closing images to ensure proper resource management.
- Add error handling for potential PIL operations that might fail.
- Consider using
itertools.chain.from_iterablefor a more concise list flattening.
Here's a suggested refactoring:
from contextlib import ExitStack
import itertools
def write_img_2_long_img(self, long_img_path, album: Optional[JmAlbumDetail], photo: Optional[JmPhotoDetail]) -> List[str]:
img_dir_items = [self.option.decide_image_save_dir(photo)] if album is None else [self.option.decide_image_save_dir(p) for p in album]
img_paths = sorted(itertools.chain.from_iterable(map(files_of_dir, img_dir_items)))
with ExitStack() as stack:
images = [stack.enter_context(self.open_image(path)) for path in img_paths]
if not images:
self.log("No images found to process", 'warning')
return []
try:
resample_method = Image.Resampling.LANCZOS
except AttributeError:
resample_method = Image.LANCZOS
min_img_width = min(img.width for img in images)
total_height = sum(int(img.height * min_img_width / img.width) for img in images)
with Image.new('RGB', (min_img_width, total_height)) as long_img:
y_offset = 0
for img in images:
if img.width > min_img_width:
img = img.resize((min_img_width, int(img.height * min_img_width / img.width)),
resample=resample_method)
long_img.paste(img, (0, y_offset))
y_offset += img.height
try:
long_img.save(long_img_path)
except IOError as e:
self.log(f"Failed to save long image: {e}", 'error')
return []
return img_paths
def open_image(self, img_path: str):
try:
return Image.open(img_path)
except IOError as e:
self.log(f"Failed to open image {img_path}: {e}", 'error')
return NoneThis refactoring improves resource management, error handling, and code conciseness.
🧰 Tools
🪛 Ruff
841-841:
JmAlbumDetailmay be undefined, or defined from star imports(F405)
841-841:
JmPhotoDetailmay be undefined, or defined from star imports(F405)
841-841:
Listmay be undefined, or defined from star imports(F405)
850-850:
files_of_dirmay be undefined, or defined from star imports(F405)
| def open_images(self, img_paths: List[str]): | ||
| images = [] | ||
| for img_path in img_paths: | ||
| try: | ||
| img = Image.open(img_path) | ||
| images.append(img) | ||
| except IOError as e: | ||
| self.log(f"Failed to open image {img_path}: {e}", 'error') | ||
| return images | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve resource management in open_images
The current implementation of open_images is functional but can be improved:
- Consider using a generator to yield images as they are opened, which can be more memory-efficient for large numbers of images.
- Use a context manager to ensure images are properly closed after use.
- Provide more detailed error information, including the specific exception type.
Here's a suggested improvement:
from contextlib import contextmanager
from typing import Iterator, List
@contextmanager
def open_image(self, img_path: str):
try:
img = Image.open(img_path)
yield img
except Exception as e:
self.log(f"Failed to open image {img_path}: {type(e).__name__}: {e}", 'error')
yield None
finally:
if 'img' in locals() and img is not None:
img.close()
def open_images(self, img_paths: List[str]) -> Iterator[Optional[Image.Image]]:
for img_path in img_paths:
with self.open_image(img_path) as img:
if img is not None:
yield imgThis implementation provides better resource management, more detailed error logging, and allows for more efficient memory usage when processing large numbers of images.
🧰 Tools
🪛 Ruff
880-880:
Listmay be undefined, or defined from star imports(F405)
884-884:
Imagemay be undefined, or defined from star imports(F405)
There was a problem hiding this comment.
Actionable comments posted: 4
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- src/jmcomic/jm_plugin.py (1 hunks)
🧰 Additional context used
🪛 Ruff
src/jmcomic/jm_plugin.py
806-806:
JmPhotoDetailmay be undefined, or defined from star imports(F405)
807-807:
JmAlbumDetailmay be undefined, or defined from star imports(F405)
815-815:
jm_logmay be undefined, or defined from star imports(F405)
818-818:
PIL.Imageimported but unused; consider usingimportlib.util.find_specto test for availability(F401)
829-829:
DirRulemay be undefined, or defined from star imports(F405)
841-841:
JmAlbumDetailmay be undefined, or defined from star imports(F405)
841-841:
JmPhotoDetailmay be undefined, or defined from star imports(F405)
841-841:
Listmay be undefined, or defined from star imports(F405)
850-850:
files_of_dirmay be undefined, or defined from star imports(F405)
881-881:
Listmay be undefined, or defined from star imports(F405)
885-885:
Imagemay be undefined, or defined from star imports(F405)
892-892:
Optionalmay be undefined, or defined from star imports(F405)
893-893:
fix_filepathmay be undefined, or defined from star imports(F405)
894-894:
mkdir_if_not_existsmay be undefined, or defined from star imports(F405)
🔇 Additional comments (2)
src/jmcomic/jm_plugin.py (2)
802-804: LGTM: Class definition and plugin_keyThe
LongImgPluginclass is correctly defined as a subclass ofJmOptionPlugin, and theplugin_keyis appropriately set to 'long_img'.
802-896: Overall assessment of LongImgPluginThe
LongImgPluginclass is well-implemented and provides a useful functionality for merging multiple images into a single long image. It handles both single photo and album inputs, and effectively uses the PIL library for image processing.Key strengths:
- Clear and descriptive method names
- Handling of both single photo and album inputs
- Efficient image merging algorithm
Areas for improvement:
- Enhanced error handling, especially for file operations
- Better code organization (e.g., moving imports to the top of the file)
- Improved memory efficiency in image processing
- More consistent use of parameters across plugins
Implementing the suggested changes will further improve the robustness, efficiency, and maintainability of this plugin. Great job overall!
🧰 Tools
🪛 Ruff
806-806:
JmPhotoDetailmay be undefined, or defined from star imports(F405)
807-807:
JmAlbumDetailmay be undefined, or defined from star imports(F405)
815-815:
jm_logmay be undefined, or defined from star imports(F405)
818-818:
PIL.Imageimported but unused; consider usingimportlib.util.find_specto test for availability(F401)
829-829:
DirRulemay be undefined, or defined from star imports(F405)
841-841:
JmAlbumDetailmay be undefined, or defined from star imports(F405)
841-841:
JmPhotoDetailmay be undefined, or defined from star imports(F405)
841-841:
Listmay be undefined, or defined from star imports(F405)
850-850:
files_of_dirmay be undefined, or defined from star imports(F405)
881-881:
Listmay be undefined, or defined from star imports(F405)
885-885:
Imagemay be undefined, or defined from star imports(F405)
892-892:
Optionalmay be undefined, or defined from star imports(F405)
893-893:
fix_filepathmay be undefined, or defined from star imports(F405)
894-894:
mkdir_if_not_existsmay be undefined, or defined from star imports(F405)
| def invoke(self, | ||
| photo: JmPhotoDetail = None, | ||
| album: JmAlbumDetail = None, | ||
| downloader=None, | ||
| img_dir=None, | ||
| filename_rule='Pid', | ||
| delete_original_file=False, | ||
| **kwargs, | ||
| ): | ||
| if photo is None and album is None: | ||
| jm_log('wrong_usage', 'long_img必须运行在after_photo或after_album时') | ||
|
|
||
| try: | ||
| from PIL import Image | ||
| except ImportError: | ||
| self.warning_lib_not_install('PIL') | ||
| return | ||
|
|
||
| self.delete_original_file = delete_original_file | ||
|
|
||
| # 处理文件夹配置 | ||
| img_dir = self.get_img_dir(img_dir) | ||
|
|
||
| # 处理生成的长图文件的路径 | ||
| filename = DirRule.apply_rule_directly(album, photo, filename_rule) | ||
|
|
||
| # 长图路径 | ||
| long_img_path = os.path.join(img_dir, f'{filename}.png') | ||
|
|
||
| # 调用 PIL 把 photo_dir 下的所有图片合并为长图 | ||
| img_path_ls = self.write_img_2_long_img(long_img_path, album, photo) | ||
| self.log(f'Convert Successfully: JM{album or photo} → {long_img_path}') | ||
|
|
||
| # 执行删除 | ||
| self.execute_deletion(img_path_ls) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve error handling and code organization in the invoke method
- Consider providing a more specific error message when both
photoandalbumare None. - Move the PIL import to the top of the file for better code organization.
- Use the
delete_original_fileparameter consistently with other plugins.
Here's a suggested improvement:
from PIL import Image
class LongImgPlugin(JmOptionPlugin):
plugin_key = 'long_img'
def invoke(self,
photo: Optional[JmPhotoDetail] = None,
album: Optional[JmAlbumDetail] = None,
downloader=None,
img_dir=None,
filename_rule='Pid',
delete_original_file=False,
**kwargs,
):
if photo is None and album is None:
raise ValueError("Either 'photo' or 'album' must be provided")
self.delete_original_file = delete_original_file
# Rest of the method...This change improves error handling, import organization, and consistency with other plugins.
🧰 Tools
🪛 Ruff
806-806:
JmPhotoDetailmay be undefined, or defined from star imports(F405)
807-807:
JmAlbumDetailmay be undefined, or defined from star imports(F405)
815-815:
jm_logmay be undefined, or defined from star imports(F405)
818-818:
PIL.Imageimported but unused; consider usingimportlib.util.find_specto test for availability(F401)
829-829:
DirRulemay be undefined, or defined from star imports(F405)
src/jmcomic/jm_plugin.py
Outdated
| def write_img_2_long_img(self, long_img_path, album: JmAlbumDetail, photo: JmPhotoDetail) -> List[str]: | ||
| import itertools | ||
| from PIL import Image | ||
|
|
||
| if album is None: | ||
| img_dir_items = [self.option.decide_image_save_dir(photo)] | ||
| else: | ||
| img_dir_items = [self.option.decide_image_save_dir(photo) for photo in album] | ||
|
|
||
| img_paths = itertools.chain(*map(files_of_dir, img_dir_items)) | ||
| img_paths = filter(lambda x: not x.startswith('.'), img_paths) # 过滤系统文件 | ||
| img_paths = sorted(img_paths) | ||
|
|
||
| images = self.open_images(img_paths) | ||
|
|
||
| try: | ||
| resample_method = Image.Resampling.LANCZOS | ||
| except AttributeError: | ||
| resample_method = Image.LANCZOS | ||
|
|
||
| min_img_width = min(img.width for img in images) | ||
| total_height = 0 | ||
| for i, img in enumerate(images): | ||
| if img.width > min_img_width: | ||
| images[i] = img.resize((min_img_width, int(img.height * min_img_width / img.width)), | ||
| resample=resample_method) | ||
| total_height += images[i].height | ||
|
|
||
| long_img = Image.new('RGB', (min_img_width, total_height)) | ||
| y_offset = 0 | ||
| for img in images: | ||
| long_img.paste(img, (0, y_offset)) | ||
| y_offset += img.height | ||
|
|
||
| long_img.save(long_img_path) | ||
| for img in images: | ||
| img.close() | ||
|
|
||
| return img_paths |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance the write_img_2_long_img method
- Replace the list comprehension for closing images with a simple for loop.
- Add error handling when saving the long image.
- Add type hinting for the return value.
Here's a suggested improvement:
def write_img_2_long_img(self, long_img_path, album: Optional[JmAlbumDetail], photo: Optional[JmPhotoDetail]) -> List[str]:
# ... (rest of the method remains the same)
try:
long_img.save(long_img_path)
except IOError as e:
self.log(f"Failed to save long image: {e}", 'error')
return []
for img in images:
img.close()
return img_pathsThese changes improve readability, error handling, and type clarity.
🧰 Tools
🪛 Ruff
841-841:
JmAlbumDetailmay be undefined, or defined from star imports(F405)
841-841:
JmPhotoDetailmay be undefined, or defined from star imports(F405)
841-841:
Listmay be undefined, or defined from star imports(F405)
850-850:
files_of_dirmay be undefined, or defined from star imports(F405)
| def open_images(self, img_paths: List[str]): | ||
| images = [] | ||
| for img_path in img_paths: | ||
| try: | ||
| img = Image.open(img_path) | ||
| images.append(img) | ||
| except IOError as e: | ||
| self.log(f"Failed to open image {img_path}: {e}", 'error') | ||
| return images |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Optimize the open_images method
Consider the following improvements:
- Use a context manager to ensure proper image closure.
- Enhance error handling to catch specific PIL exceptions.
- Convert the method to a generator for better memory efficiency.
Here's a suggested refactoring:
from PIL import UnidentifiedImageError
def open_images(self, img_paths: List[str]) -> Iterator[Optional[Image.Image]]:
for img_path in img_paths:
try:
with Image.open(img_path) as img:
yield img.copy() # yield a copy to keep the image in memory after the context manager closes
except (IOError, UnidentifiedImageError) as e:
self.log(f"Failed to open image {img_path}: {e}", 'error')
yield NoneThis implementation provides better resource management, more specific error handling, and allows for more efficient memory usage when processing large numbers of images.
🧰 Tools
🪛 Ruff
881-881:
Listmay be undefined, or defined from star imports(F405)
885-885:
Imagemay be undefined, or defined from star imports(F405)
| @staticmethod | ||
| def get_img_dir(img_dir: Optional[str]) -> str: | ||
| img_dir = fix_filepath(img_dir or os.getcwd()) | ||
| mkdir_if_not_exists(img_dir) | ||
| return img_dir |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refine the get_img_dir method
Consider the following suggestions:
- Remove the
@staticmethoddecorator as it's not necessary for this method. - Add error handling for directory creation.
Here's a proposed improvement:
def get_img_dir(self, img_dir: Optional[str]) -> str:
img_dir = fix_filepath(img_dir or os.getcwd())
try:
mkdir_if_not_exists(img_dir)
except OSError as e:
self.log(f"Failed to create directory {img_dir}: {e}", 'error')
raise
return img_dirThis change improves error handling and removes the unnecessary static method designation.
🧰 Tools
🪛 Ruff
892-892:
Optionalmay be undefined, or defined from star imports(F405)
893-893:
fix_filepathmay be undefined, or defined from star imports(F405)
894-894:
mkdir_if_not_existsmay be undefined, or defined from star imports(F405)
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (3)
src/jmcomic/jm_plugin.py (3)
814-822: Move PIL import to the top of the fileImporting
ImagefromPILinside theinvokemethod can lead to repeated imports if the method is called multiple times. Move the import to the top of the file for better performance and readability.Apply this change:
+from PIL import Image class LongImgPlugin(JmOptionPlugin): plugin_key = 'long_img' def invoke(self, photo: JmPhotoDetail = None, album: JmAlbumDetail = None, downloader=None, img_dir=None, filename_rule='Pid', delete_original_file=False, **kwargs, ): - try: - from PIL import Image - except ImportError: - self.warning_lib_not_install('PIL') - returnHandle the
ImportErrorat the module level if necessary.🧰 Tools
🪛 Ruff
815-815:
jm_logmay be undefined, or defined from star imports(F405)
818-818:
PIL.Imageimported but unused; consider usingimportlib.util.find_specto test for availability(F401)
868-875: Use context managers to manage image resourcesConsider using a context manager for
long_imgto ensure resources are properly managed, and ensurelong_imgis closed after saving.Refactor the code:
-long_img = Image.new('RGB', (min_img_width, total_height)) +with Image.new('RGB', (min_img_width, total_height)) as long_img: y_offset = 0 for img in images: long_img.paste(img, (0, y_offset)) y_offset += img.height long_img.save(long_img_path)This change ensures that
long_imgis properly closed after use.
880-889: Annotate missing return type inopen_imagesThe
open_imagesmethod lacks a return type annotation. Adding it improves code clarity and type checking.Update the function signature:
def open_images(self, img_paths: List[str]) -> List[Image.Image]:🧰 Tools
🪛 Ruff
880-880:
Listmay be undefined, or defined from star imports(F405)
884-884:
Imagemay be undefined, or defined from star imports(F405)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- src/jmcomic/jm_plugin.py (1 hunks)
🧰 Additional context used
🪛 Ruff
src/jmcomic/jm_plugin.py
806-806:
JmPhotoDetailmay be undefined, or defined from star imports(F405)
807-807:
JmAlbumDetailmay be undefined, or defined from star imports(F405)
815-815:
jm_logmay be undefined, or defined from star imports(F405)
818-818:
PIL.Imageimported but unused; consider usingimportlib.util.find_specto test for availability(F401)
829-829:
DirRulemay be undefined, or defined from star imports(F405)
841-841:
JmAlbumDetailmay be undefined, or defined from star imports(F405)
841-841:
JmPhotoDetailmay be undefined, or defined from star imports(F405)
841-841:
Listmay be undefined, or defined from star imports(F405)
850-850:
files_of_dirmay be undefined, or defined from star imports(F405)
880-880:
Listmay be undefined, or defined from star imports(F405)
884-884:
Imagemay be undefined, or defined from star imports(F405)
891-891:
Optionalmay be undefined, or defined from star imports(F405)
892-892:
fix_filepathmay be undefined, or defined from star imports(F405)
893-893:
mkdir_if_not_existsmay be undefined, or defined from star imports(F405)
🔇 Additional comments (2)
src/jmcomic/jm_plugin.py (2)
802-804: ClassLongImgPluginadded successfullyThe
LongImgPluginclass extendsJmOptionPluginand is properly initialized with the requiredplugin_key. The class definition aligns with the existing plugin architecture.
891-894:⚠️ Potential issueRemove
@staticmethoddecorator fromget_img_dirThe method
get_img_diris using instance methods and therefore should not be a static method. Removing the@staticmethoddecorator ensures the method functions correctly.Adjust the code:
-@staticmethod def get_img_dir(self, img_dir: Optional[str]) -> str:Likely invalid or redundant comment.
🧰 Tools
🪛 Ruff
891-891:
Optionalmay be undefined, or defined from star imports(F405)
892-892:
fix_filepathmay be undefined, or defined from star imports(F405)
893-893:
mkdir_if_not_existsmay be undefined, or defined from star imports(F405)
| def invoke(self, | ||
| photo: JmPhotoDetail = None, | ||
| album: JmAlbumDetail = None, | ||
| downloader=None, | ||
| img_dir=None, | ||
| filename_rule='Pid', | ||
| delete_original_file=False, | ||
| **kwargs, | ||
| ): |
There was a problem hiding this comment.
Improve error handling when no input is provided
In the invoke method, when both photo and album are None, it logs an error using jm_log but continues execution. Consider raising a ValueError to prevent unintended behavior.
Adjust the code as follows:
def invoke(self,
photo: JmPhotoDetail = None,
album: JmAlbumDetail = None,
downloader=None,
img_dir=None,
filename_rule='Pid',
delete_original_file=False,
**kwargs,
):
if photo is None and album is None:
- jm_log('wrong_usage', 'long_img必须运行在after_photo或after_album时')
+ raise ValueError('long_img插件必须在after_photo或after_album时调用，必须提供photo或album参数')📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def invoke(self, | |
| photo: JmPhotoDetail = None, | |
| album: JmAlbumDetail = None, | |
| downloader=None, | |
| img_dir=None, | |
| filename_rule='Pid', | |
| delete_original_file=False, | |
| **kwargs, | |
| ): | |
| def invoke(self, | |
| photo: JmPhotoDetail = None, | |
| album: JmAlbumDetail = None, | |
| downloader=None, | |
| img_dir=None, | |
| filename_rule='Pid', | |
| delete_original_file=False, | |
| **kwargs, | |
| ): | |
| if photo is None and album is None: | |
| raise ValueError('long_img插件必须在after_photo或after_album时调用，必须提供photo或album参数') |
🧰 Tools
🪛 Ruff
806-806:
JmPhotoDetailmay be undefined, or defined from star imports(F405)
807-807:
JmAlbumDetailmay be undefined, or defined from star imports(F405)
| img_paths = itertools.chain(*map(files_of_dir, img_dir_items)) | ||
| img_paths = filter(lambda x: not x.startswith('.'), img_paths) # 过滤系统文件 | ||
|
|
||
| images = self.open_images(img_paths) |
There was a problem hiding this comment.
Convert img_paths to a list to prevent exhaustion
img_paths is an iterator created using itertools.chain and filtered. Since it is used multiple times later in the code, converting it to a list ensures the iterator is not exhausted prematurely.
Modify the code as follows:
img_paths = itertools.chain(*map(files_of_dir, img_dir_items))
img_paths = filter(lambda x: not x.startswith('.'), img_paths) # 过滤系统文件
+img_paths = list(img_paths)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| img_paths = itertools.chain(*map(files_of_dir, img_dir_items)) | |
| img_paths = filter(lambda x: not x.startswith('.'), img_paths) # 过滤系统文件 | |
| images = self.open_images(img_paths) | |
| img_paths = itertools.chain(*map(files_of_dir, img_dir_items)) | |
| img_paths = filter(lambda x: not x.startswith('.'), img_paths) # 过滤系统文件 | |
| img_paths = list(img_paths) | |
| images = self.open_images(img_paths) |
🧰 Tools
🪛 Ruff
850-850:
files_of_dirmay be undefined, or defined from star imports(F405)
| min_img_width = min(img.width for img in images) | ||
| total_height = 0 | ||
| for i, img in enumerate(images): |
There was a problem hiding this comment.
Handle empty image list gracefully
When images is empty, min(img.width for img in images) will raise a ValueError. Add a check to handle the case when no images are found.
Update the code:
images = self.open_images(img_paths)
+if not images:
+ self.log("No images found to process", 'warning')
+ return []
try:
resample_method = Image.Resampling.LANCZOSCommittable suggestion was skipped due to low confidence.
| for img in images: | ||
| img.close() | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Ensure all opened images are closed
Currently, images are manually closed in a loop after processing. Using context managers when opening images can ensure they are properly closed even if an exception occurs.
Modify the open_images method:
def open_images(self, img_paths: List[str]):
- images = []
- for img_path in img_paths:
- try:
- img = Image.open(img_path)
- images.append(img)
- except IOError as e:
- self.log(f"Failed to open image {img_path}: {e}", 'error')
- return images
+ images = []
+ for img_path in img_paths:
+ try:
+ with Image.open(img_path) as img:
+ images.append(img.copy())
+ except IOError as e:
+ self.log(f"Failed to open image {img_path}: {e}", 'error')
+ return imagesThis ensures images are properly closed after use.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| for img in images: | |
| img.close() | |
| def open_images(self, img_paths: List[str]): | |
| images = [] | |
| for img_path in img_paths: | |
| try: | |
| with Image.open(img_path) as img: | |
| images.append(img.copy()) | |
| except IOError as e: | |
| self.log(f"Failed to open image {img_path}: {e}", 'error') | |
| return images |
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
long_imgplugin and its parameters.