feat: add generator options #125
Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds comprehensive generator options for PDF credential customization, including support for individual fonts per text element, uppercase text transformation, character spacing, and improved multiline context name handling. The version is bumped to 0.3.1-rc1.
Key Changes:
- Refactored font registration to support individual fonts for name, context name, and issue date
- Added uppercase transformation options with instance-level settings fallback
- Enhanced multiline context name support with
template_multilineoption (replacingtemplate_two_lines) - Introduced character spacing option for issue dates
Reviewed changes
Copilot reviewed 6 out of 7 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| learning_credentials/generators.py | Core implementation of new generator options including font registration refactoring, uppercase support, character spacing, and improved multiline handling |
| tests/test_generators.py | Comprehensive test coverage for new font registration behavior, uppercase transformations, character spacing, and template selection logic |
| learning_credentials/compat.py | Updated course name retrieval to use cert_name_long field from course overviews instead of learning sequences API |
| pyproject.toml | Version bump to 0.3.1-rc1 and addition of FBT001 to linting ignore list |
| tests/test_processors.py | Removed now-redundant noqa comment for FBT001 |
| CHANGELOG.rst | Documented new features and modifications for version 0.3.1 |
| uv.lock | Auto-generated dependency marker updates and version synchronization |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| try: | ||
| registerFont(TTFont(font_name, CredentialAsset.get_asset_by_slug(font_name))) | ||
| except (FontError, FontNotFoundError, TTFError): | ||
| log.exception("Error registering font %s", font_name) | ||
| else: | ||
| return font_name |
There was a problem hiding this comment.
When font registration fails, the function logs an exception but silently returns None, which causes the code to fall back to Helvetica. However, users might not be aware their custom font failed to load. Consider also logging a warning at the point where the fallback occurs (lines 95, 101, 112, 131) so users can see in logs that their custom font was ignored and the default was used instead.
tests/test_generators.py
Outdated
| def test_register_font_without_custom_font(mock_get_asset_by_slug: Mock): | ||
| """Test the _register_font falls back to the default font when no custom font is specified.""" | ||
| options = {} | ||
| assert _register_font(options) == "Helvetica" | ||
| assert _register_font(options) is None |
There was a problem hiding this comment.
The test is passing an empty dictionary to _register_font, but the function signature has changed to accept a font_name parameter directly (not an options dict). The test should pass None or an empty string instead.
This: 1. Replaces `template_two_lines` with `template_multiline`. 2. Adds support for `\n` in context names.
Completion Aggregator (dependency) does not support Django 5.2 at the moment.
Agrendalath
force-pushed
the
agrendalath/add-generator-options
branch
3 times, most recently
from
fa6eb57 to
10f3885
Compare
BREAKING CHANGE: Flat-structured options are no longer supported by the `generate_pdf_credential` generator.
Agrendalath
force-pushed
the
agrendalath/add-generator-options
branch
from
10f3885 to
45c6ebb
Compare
Merge checklist:
Check off if complete or not applicable: