Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0f1ba3e09d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if len(web_files) == 0: | ||
| raise RuntimeError("ZIP archive did not contain any supported logo image files.") |
There was a problem hiding this comment.
The importer currently raises an error whenever no web/logo files are found, even if the archive contains valid images under print/ or logos/ that were already parsed into print_files. This makes print-only logo packs impossible to import, despite the surrounding import flow explicitly supporting print-targeted assets, so users with label-only packs will always get a hard failure.
Useful? React with 👍 / 👎.
| print_files: list[str] = [] | ||
|
|
||
| if source_web_dir is not None: | ||
| for source_file in sorted(source_web_dir.rglob("*.png")): |
There was a problem hiding this comment.
The remote tree scan treats .png case-insensitively (path.lower().endswith(".png")), but the archive copy step uses rglob("*.png"), which is case-sensitive on Linux. Repos that contain files like LOGO.PNG will be counted in the signature and remote counts but never copied locally, leaving the runtime pack incomplete while later sync checks still report it as current.
Useful? React with 👍 / 👎.
| dest_file = web_dir / source_file.name | ||
| shutil.copy2(source_file, dest_file) |
There was a problem hiding this comment.
When syncing from GitHub, every source file is flattened to source_file.name, so two different paths like web/brandA/logo.png and web/brandB/logo.png will overwrite each other in the runtime pack. This silently drops logos and can make suggestions point to the wrong image for repositories that organize assets in subdirectories.
Useful? React with 👍 / 👎.
akira69
force-pushed
the
review/857-on-860
branch
from
b555eaa to
837aafe
Compare
akira69
force-pushed
the
review/872-on-857
branch
2 times, most recently
from
dea6ca6 to
3910c1f
Compare
akira69
force-pushed
the
review/857-on-860
branch
from
06c9825 to
d127692
Compare
akira69
force-pushed
the
review/872-on-857
branch
from
3910c1f to
992a00e
Compare
akira69
force-pushed
the
review/857-on-860
branch
from
fda598b to
741d993
Compare
akira69
force-pushed
the
review/872-on-857
branch
from
b541a65 to
69de241
Compare
1 participant
This is a fork-only stacked review PR for upstream Donkie/Spoolman#872.
Use this PR for true stacked review diff only.
Chain:
review/872-on-857Notes:
feat/manufacturer-logo-github-sync-upstream-splitbranch tree.masterso the upstream PR stays clean.