fix: allow PDF/PNG generation when dont_generate_typst is True#577
Draft
majiayu000 wants to merge 4 commits intorendercv:mainfrom
Draft
fix: allow PDF/PNG generation when dont_generate_typst is True#577majiayu000 wants to merge 4 commits intorendercv:mainfrom
majiayu000 wants to merge 4 commits intorendercv:mainfrom
Conversation
When dont_generate_typst was set to True, the entire PDF/PNG generation was disabled because generate_typst returned None and generate_pdf/png checked for None typst_path. Now generate_typst creates a temporary file when dont_generate_typst=True, allowing PDF/PNG compilation to proceed while not saving the .typ file to the output directory. Fixes rendercv#550
The function no longer returns None after the fix, so update the return type from `pathlib.Path | None` to `pathlib.Path`. Signed-off-by: majiayu000 <1835304752@qq.com>
Member
|
Are we sure those temp dirs get removed at the end of the run? |
When dont_generate_typst is True, generate_typst() creates a temporary directory for the typst file. Previously, this temp directory was never cleaned up, leading to leftover files in the system temp directory. Changes: - Modified generate_typst() to return a tuple (path, is_temporary) so callers know when cleanup is needed - Added cleanup logic in run_rendercv() using a finally block to ensure temp directories are removed even if an error occurs - Updated tests to verify the cleanup behavior This addresses the review comment in PR rendercv#577 about temp directory cleanup. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Contributor
Author
|
Good catch! You're absolutely right - the temp directories were not being cleaned up. I've pushed a fix that:
The temp directory is now properly cleaned up after the PDF/PNG compilation is complete. All 444 tests pass. Thanks for the review! |
Move `import shutil` from inside test functions to the top of the file to comply with ruff check rule PLC0415 (import should be at the top-level of a file). 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
3 tasks
|
OS's temp dir could also be used. |
sinaatalay
force-pushed
the
main
branch
3 times, most recently
from
cff31ca to
2b08030
Compare
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
3 participants
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.
Summary
dont_generate_typst: truewould also disable PDF and PNG generationChanges
generate_typst()to return a temp file path instead of None whendont_generate_typst=Truegenerate_pdf()andgenerate_png()to always expect a valid typst pathdont_generate_typst=TrueFixes #550