Add filename options - #5
Open
alesf wants to merge 2 commits into
Open
Conversation
- filename presets and custom template options in the UI, - updated backend to handle filename formatting based on user selection, - added validation for custom filename templates, - enhanced metadata extraction to include publication year, - implemented unit tests for filename formatting functionality
📝 WalkthroughWalkthroughChangesFilename configuration
Estimated code review effort: 4 (Complex) | ~45 minutes Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant Browser
participant processFile
participant process_sse
participant format_filename
participant EPUBOutput
Browser->>processFile: Select filename format and template
processFile->>process_sse: Send filename settings and original filename
process_sse->>process_sse: Validate format and template
process_sse->>format_filename: Pass settings and extracted metadata
format_filename->>EPUBOutput: Return sanitized EPUB filename
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@metadata_handler.py`:
- Around line 338-356: Update the custom filename validation around
parsed_template and fields so bare or whitespace-only placeholders are treated
as unsupported fields rather than filtered out. Ensure format_filename() raises
the existing ValueError for these templates before template.format runs, while
preserving validation of named fields against FILENAME_TEMPLATE_FIELDS.
In `@test_metadata_handler.py`:
- Around line 8-78: The FormatFilenameTests suite lacks regression coverage for
bare-brace custom templates. Add a test alongside
test_custom_template_rejects_formatting_options that calls
format_filename("Book", "Writer", "custom", "{}") and asserts it raises
ValueError, matching the validation behavior implemented in format_filename.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: aa369e76-f806-4a5c-ba9d-fa1d1c9fcec8
📒 Files selected for processing (8)
README.mdapp.pyepub_processor.pymetadata_handler.pystatic/app.jsstatic/style.csstemplates/index.htmltest_metadata_handler.py
Comment on lines
+338
to
+356
| if not template.strip(): | ||
| raise ValueError("Custom filename template cannot be empty") | ||
| parsed_template = list(string.Formatter().parse(template)) | ||
| if any(format_spec or conversion for _, _, format_spec, conversion in parsed_template): | ||
| raise ValueError("Filename template fields do not support formatting options") | ||
| fields = {field_name for _, field_name, _, _ in parsed_template if field_name} | ||
| unsupported = fields - FILENAME_TEMPLATE_FIELDS | ||
| if unsupported: | ||
| names = ', '.join(sorted(unsupported)) | ||
| raise ValueError(f"Unknown filename template field: {names}") | ||
| name = template.format( | ||
| title=title, | ||
| author=author, | ||
| year=year, | ||
| series=series, | ||
| series_index=series_index, | ||
| language=language, | ||
| original=original, | ||
| ) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Repo files matching metadata_handler.py / app.py / epub_processor.py:"
fd -a 'metadata_handler\.py|app\.py|epub_processor\.py' . | sed 's#^\./##'
echo
echo "Python version and string.Formatter().parse behavior:"
python3 --version
python3 - <<'PY'
import string
for template in ["{}", "{ }", "{title}", "{0}", "{title[foo]}"]:
parsed = list(string.Formatter().parse(template))
print(template, parsed)
fields = {field_name for _, field_name, _, _ in parsed if field_name}
print(" filtered fields={}", fields)
PYRepository: b1rdmania/epubkit
Length of output: 742
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "metadata_handler.py relevant section:"
sed -n '300,380p' metadata_handler.py | nl -ba -v300
echo
echo "app.py relevant sections:"
sed -n '150,190p' app.py | nl -ba -v150
rg -n "custom|filename|ValueError|IndexError|except" app.py
echo
echo "epub_processor.py relevant sections:"
sed -n '410,450p' epub_processor.py | nl -ba -v410
rg -n "custom|filename|ValueError|except Exception|Metadata" epub_processor.py
echo
echo "All occurrences of filename template logic:"
rg -n "FILENAME_TEMPLATE_FIELDS|filename template|custom filename|format\\(" metadata_handler.py app.py epub_processor.py test_metadata_handler.pyRepository: b1rdmania/epubkit
Length of output: 233
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "metadata_handler.py relevant section:"
sed -n '300,380p' metadata_handler.py | awk '{printf "%5d %s\n", NR+299, $0}'
echo
echo "app.py relevant sections and occurrences:"
sed -n '150,190p' app.py | awk '{printf "%5d %s\n", NR+149, $0}'
rg -n "custom|filename|ValueError|IndexError|except" app.py
echo
echo "epub_processor.py relevant sections and occurrences:"
sed -n '410,450p' epub_processor.py | awk '{printf "%5d %s\n", NR+409, $0}'
rg -n "custom|filename|ValueError|except Exception|Metadata" epub_processor.py
echo
echo "All occurrences of filename template logic:"
rg -n "FILENAME_TEMPLATE_FIELDS|filename template|custom filename|format\\(" metadata_handler.py app.py epub_processor.py test_metadata_handler.pyRepository: b1rdmania/epubkit
Length of output: 10658
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Python formatting behavior for bare placeholder templates during validation and formatting:"
python3 - <<'PY'
import string
from pathlib import Path
FILENAME_TEMPLATE_FIELDS = {'title', 'author', 'year', 'series', 'series_index', 'language', 'original'}
for template in ["{}", "{ }", "{title}"]:
parsed = list(string.Formatter().parse(template))
fields = {field_name for _, field_name, _, _ in parsed if field_name}
unsupported = fields - FILENAME_TEMPLATE_FIELDS
try:
name = template.format(
title="title",
author="author",
year="2026",
series="series",
series_index="1",
language="en",
original=Path("original.epub").name,
)
print(template, "parsed=", parsed, "fields=", fields, "unsupported=", unsupported, "name=", name)
except Exception as e:
print(template, "parsed=", parsed, "fields=", fields, "unsupported=", unsupported, "raised", type(e).__name__, str(e))
print()
print("Literal blank field as {}")
try:
"{} {}".format(title="t", author="a")
except Exception as e:
print(type(e).__name__, str(e))
PYRepository: b1rdmania/epubkit
Length of output: 664
Reject bare field placeholders in custom filename templates.
string.Formatter().parse() treats {} and { } as fields with an empty or whitespace field name. The current if field_name, filter excludes empty names, so unsupported stays empty; template.format(title=...) then raises IndexError because format_filename() only supplies keyword arguments. Treat None field names as unknown fields and raise ValueError, so malformed templates are rejected before processing instead of an unhandled IndexError.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@metadata_handler.py` around lines 338 - 356, Update the custom filename
validation around parsed_template and fields so bare or whitespace-only
placeholders are treated as unsupported fields rather than filtered out. Ensure
format_filename() raises the existing ValueError for these templates before
template.format runs, while preserving validation of named fields against
FILENAME_TEMPLATE_FIELDS.
Comment on lines
+8
to
+78
| class FormatFilenameTests(unittest.TestCase): | ||
| def test_presets(self): | ||
| self.assertEqual( | ||
| format_filename("The Book", "A. Writer", "original", original_filename="upload.epub"), | ||
| "upload.epub", | ||
| ) | ||
| self.assertEqual( | ||
| format_filename("The Book", "A. Writer", "title-author"), | ||
| "The Book - A. Writer.epub", | ||
| ) | ||
| self.assertEqual( | ||
| format_filename("The Book", "A. Writer", "author-title"), | ||
| "A. Writer - The Book.epub", | ||
| ) | ||
| self.assertEqual( | ||
| format_filename("The Book", "A. Writer", "title"), | ||
| "The Book.epub", | ||
| ) | ||
|
|
||
| def test_custom_template_supports_metadata_and_original_name(self): | ||
| self.assertEqual( | ||
| format_filename( | ||
| "The Book", | ||
| "A/Writer", | ||
| "custom", | ||
| "{year} - {title} - {author} [{original}]", | ||
| "source.epub", | ||
| "2026", | ||
| ), | ||
| "2026 - The Book - A-Writer [source].epub", | ||
| ) | ||
|
|
||
| def test_custom_template_supports_series_and_language(self): | ||
| self.assertEqual( | ||
| format_filename( | ||
| "The Book", | ||
| "A. Writer", | ||
| "custom", | ||
| "{series} {series_index} - {title} ({language})", | ||
| series="Saga", | ||
| series_index="2", | ||
| language="en", | ||
| ), | ||
| "Saga 2 - The Book (en).epub", | ||
| ) | ||
|
|
||
| def test_missing_metadata_and_unsafe_names_fall_back_safely(self): | ||
| self.assertEqual(format_filename("", "", "title-author"), "optimized.epub") | ||
| self.assertEqual( | ||
| format_filename("", "", "original", original_filename="../../.epub"), | ||
| "optimized.epub", | ||
| ) | ||
|
|
||
| def test_custom_template_rejects_unknown_fields(self): | ||
| with self.assertRaisesRegex(ValueError, "Unknown filename template field"): | ||
| format_filename("Book", "Writer", "custom", "{publisher}") | ||
|
|
||
| def test_custom_template_rejects_formatting_options(self): | ||
| with self.assertRaisesRegex(ValueError, "do not support formatting options"): | ||
| format_filename("Book", "Writer", "custom", "{title:>20}") | ||
|
|
||
| def test_extract_metadata_includes_publication_year(self): | ||
| opf = etree.fromstring(b""" | ||
| <package xmlns="http://www.idpf.org/2007/opf"> | ||
| <metadata xmlns:dc="http://purl.org/dc/elements/1.1/"> | ||
| <dc:title>Book</dc:title> | ||
| <dc:date>2026-07-31</dc:date> | ||
| </metadata> | ||
| </package> | ||
| """) | ||
| self.assertEqual(extract_metadata(etree.ElementTree(opf))["year"], "2026") |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Good coverage of presets, custom templates, and fallbacks. Add a case for bare-brace templates.
The tests validate presets, series/language fields, unsafe-name fallback, and rejection of unknown fields and formatting options. None of them exercise a template with an auto-numbered placeholder such as "{}", which is the input that triggers the uncaught IndexError described in the metadata_handler.py review. Add a regression test asserting that format_filename("Book", "Writer", "custom", "{}") raises ValueError once the fix in metadata_handler.py lands.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@test_metadata_handler.py` around lines 8 - 78, The FormatFilenameTests suite
lacks regression coverage for bare-brace custom templates. Add a test alongside
test_custom_template_rejects_formatting_options that calls
format_filename("Book", "Writer", "custom", "{}") and asserts it raises
ValueError, matching the validation behavior implemented in format_filename.
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
1 participant
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.
Add option to select output filename format.
Closes #4
Summary by CodeRabbit
New Features
Documentation
Tests