Fix macOS build, resolve clippy warnings, add Windows ARM64#22
Conversation
- Pin zip to 2.2.0 with default-features=false and deflate-miniz only, removing the lzma-sys C dependency that failed to compile on macOS 26 due to stdlib.h not found with -mmacosx-version-min=26.2 - Fix all clippy warnings across batching, helpers, scraper, types, cbz, pdf, and main (needless_borrow, io_other_error, format_in_format_args, clone_on_copy, redundant_pattern_matching, manual_flatten, and more) - Add aarch64-pc-windows-msvc target to release CI workflow
There was a problem hiding this comment.
Pull request overview
This PR improves cross-platform builds and reduces Clippy noise while simplifying portions of the scraper and writer logic, including changes intended to fix macOS ZIP/CBZ creation and add Windows ARM64 release artifacts.
Changes:
- Update
zipdependency configuration (v2.2.0, disable default features, enabledeflate-miniz) and extend the release workflow matrix withaarch64-pc-windows-msvc. - Refactor scraper helper/error conversion and simplify various scraper logic patterns (options handling, iteration,
is_none()checks). - Simplify writer directory iteration (using
.flatten()) and addDefaultforTableOfContents.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
Cargo.toml |
Updates zip crate configuration for improved compatibility (macOS build fix). |
.github/workflows/release.yml |
Adds Windows ARM64 target to release build matrix. |
src/util/main.rs |
Minor CLI/util parsing and result-handling cleanup. |
src/lib/scraper/helpers.rs |
Refactors Option/Result-to-io::Result helpers and URL/path utilities. |
src/lib/scraper/batching.rs |
Removes unnecessary references when calling helper functions. |
src/lib/scraper/scraper.rs |
Small logic simplifications around formats handling and page filtering. |
src/lib/scraper/types.rs |
Clippy allow for enum variant size + small parsing/iteration refactors. |
src/lib/scraper/mod.rs |
Adds Clippy suppression for module inception pattern. |
src/lib/writer/cbz.rs |
Simplifies directory iteration for CBZ creation. |
src/lib/writer/pdf.rs |
Adds Default for TOC and simplifies directory iteration for PDF creation. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Much appreciated! This was my first project in Rust and I have not used Rust much or done much of any software development outside of work since starting my current job, so it will probably take me a while to refresh my memory and properly review this. At a glance everything looks fine though, and if there are no issues I will merge once I get a chance to review. |
- helpers.rs: convert msg to owned String in ToResultErrorMessage to satisfy 'static bound - cbz.rs: propagate errors from File::create, file_name, seek, and read_to_end instead of unwrapping/discarding - pdf.rs: replace all unwrap() calls with proper io::Error returns; propagate insert_image error instead of silently logging - main.rs: propagate read_to_string error with ?; handle to_options() failure gracefully with stderr + exit(1)
|
@shloop No worries, and Rust is such an excellent language to work with for a command line utility like this. I'm learning it through a lot of Agentic GitOps, but applying three decades of experience working in software development and enterprise architecture across private organizations and public sector. The Rust linter I'm working on these Copilot findings for you now and applying them to this pull request. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 10 changed files in this pull request and generated 6 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 10 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 10 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
@shloop Apologies for the long PR and many commits. I had to fix more things the longer Copilot kept giving me feedback. My first commit -- or maybe this repo always had -- a page sort order issue I reproduced last night and have now fixed. There's more files and lines of code to look at now, but I'm a lot happier with this result. |
shloop
left a comment
There was a problem hiding this comment.
All looks good except for the part I commented on.
Co-authored-by: shloop <shloop@shloop.net>
This pull request introduces several improvements and fixes across the codebase, focusing on enhanced platform support, dependency updates, code simplification, bug fixes, and robustness improvements identified during code review. The most significant changes include fixing macOS builds by updating the
zipcrate configuration, adding Windows ARM64 build support, improving error handling and code clarity, fixing minor logic issues in the scraper and utility modules, and hardening the CBZ and PDF writers against silent failures and non-deterministic page ordering (Resolves #19).Platform and Dependency Updates:
zipcrate to version 2.2.0, disabled default features, and enabled thedeflate-minizfeature for more efficient and compatible CBZ/ZIP file creation. This is required for macOS compatibility (Cargo.toml).aarch64-pc-windows-msvc) as a build target in the GitHub Actions release workflow, enabling distribution for more Windows devices (.github/workflows/release.yml).Code Quality and Error Handling Improvements:
io::Error::otherand removed redundant doc comments, improving error handling throughoutsrc/lib/scraper/helpers.rs. [1] [2].next_back()instead of.last(), simplifiedgenerate_image_filenameto a singleformat!call, and removed a redundant.to_string()call on aCow<str>. [1] [2]Scraper Logic and Bug Fixes:
src/lib/scraper/batching.rsandsrc/lib/scraper/scraper.rs. [1] [2] [3]None/Somechecks that caused pages to be skipped incorrectly. Improved iteration over page children in metadata parsing by replacing a manual counter withe.text().enumerate(). [1] [2] [3]Writer Robustness (from code review):
create_cbz(src/lib/writer/cbz.rs) andcreate_pdf_internal(src/lib/writer/pdf.rs) now collect allread_direntries eagerly viacollect::<io::Result<_>>()?, propagating any entry-level I/O errors instead of silently dropping them with.flatten().00001-,00002-, etc. now reliably determines page sequence). [1] [2]File::openfailures increate_cbznow propagate asio::Errorinstead of silently skipping pages, preventing the creation of incomplete archives without any indication of failure. [1]lopdf::xobject::image()failures increate_pdf_internalnow propagate viamap_err(...)?instead of being silently skipped viaif let Ok(stream) = ..., preventing the creation of PDFs with missing pages. [1]doc.insert_image()errors now propagate with a descriptive message viaio::Error::other(...)instead of printing"error!: {name}"and continuing. [1]content.encode(), filenameinto_string(), and path.to_str()conversions now return structuredio::Errorinstead of panicking via.unwrap(). [1]Utility and Miscellaneous:
args.to_options().unwrap()insrc/util/main.rsnow matches on the result, printing to stderr and callingprocess::exit(1)on error.fs::read_to_string(file).unwrap()is replaced with?propagation. [1] [2]Defaulttrait forTableOfContentsinsrc/lib/writer/pdf.rsto satisfy Clippy'snew_without_defaultlint. [1]#[allow(clippy::large_enum_variant)]toDownloadStatusand#[allow(clippy::module_inception)]tosrc/lib/scraper/mod.rsto suppress relevant Clippy warnings. [1] [2]